559268 - nanojit: add LIR_nop

Status: RESOLVED WONTFIX

(Core Graveyard :: Nanojit, defect, P2)

Description

[Nicholas Nethercote [inactive]]

It would be useful to have LIR_nop.  Currently ExprFilter::insGuard() returns a NULL LIns* if the guard can be completely removed, and ExprFilter::insBranch() soon will too (bug 558814).  If we had LIR_nop we could return that instead and we wouldn't have to allow NULL LIns* values in certain places.

A step beyond that:  if we had LIR_nop{1,2,3,4} we would have the ability to overwrite instructions in a LIR buffer.  This might be useful.

Comment 1

[Edwin Smith]

background: LIR_nop came up in a discussion about a possible LIR_jn (jump never) instruction, which would be the result of optimizing a conditional branch that is never taken.

The idea with LIR_jn was that its still treated as a jump: you can call setTarget() for branch patching.  without that, the current NULL checks would have to be replaced by isBranch() checks, which doesn't improve things very much.
'course, if sizeof(LInsJ) is, say, 2 machine words, then LIR_nop2 could be a decent alias for it.

If the code calling insGuard() has a similar problem, would it want a LIR_xn (exit never) with an intact guard record?  not sure how far we want to carry this, given Gal's desire to never emit such a thing.

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to comment #1)
> 
> The idea with LIR_jn was that its still treated as a jump: you can call
> setTarget() for branch patching.  without that, the current NULL checks would
> have to be replaced by isBranch() checks, which doesn't improve things very
> much.

I'm hitting exactly this problem with bug 600127, due to the increased optimization it does of comparison used in guards/branches.

> 'course, if sizeof(LInsJ) is, say, 2 machine words, then LIR_nop2 could be a
> decent alias for it.

Hmm, but that doesn't indicate that you can safely call setTarget() on it.

> If the code calling insGuard() has a similar problem, would it want a LIR_xn
> (exit never) with an intact guard record?  not sure how far we want to carry
> this, given Gal's desire to never emit such a thing.

We don't subsequently call setXYZ() on a guard instruction, so it's a less pressing case.  We could just reuse LIR_jn as the nop in that case.

So I think I'll just add LIR_jn for now, as it solves a specific problem that I'm hitting now.

Comment 3

[Nicholas Nethercote [inactive]]

One downside of LIR_jn is you still end up with a label in the instruction stream, eg:

  jn label1
  ...
  label1:
  ...

And that label can screw up optimization like CSE even though it's never used.  I can see a way to fix that, though...  TM almost always uses setTarget() like this:

    branch->setTarget(lir->ins0(LIR_label));

And there's one place where it does this:

    LIns* label = lir->ins0(LIR_label);
    branch1->setTarget(label);
    branch2->setTarget(label);

These could become:

   lir->insLabel(branch);

and

   lir->insLabel(branch1);
   lir->insLabel(branch2);

In the latter case it's created two consecutive labels which were unnecessary but that's not much of a problem.

But I just looked at TR and it does much more complex things with setTarget(), erk.

Maybe instead we could make setTarget a *non-virtual* member of LirWriter (ie. one not implemented by all the pipeline stages), viz:

  lir->setTarget(branch, label);

It would check if 'branch' is NULL (or perhaps LIR_jn?) and return LIR_nop if so?

Good grief, why is this so difficult?

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to comment #3)
> 
> Maybe instead we could make setTarget a *non-virtual* member of LirWriter (ie.
> one not implemented by all the pipeline stages), viz:
> 
>   lir->setTarget(branch, label);
> 
> It would check if 'branch' is NULL (or perhaps LIR_jn?) and return LIR_nop if
> so?

Actually, its return type would be 'void', if 'branch' was NULL (or LIR_jn) it'd just do nothing.

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to comment #4)
> > 
> >   lir->setTarget(branch, label);
> 
> Actually, its return type would be 'void', if 'branch' was NULL (or LIR_jn)
> it'd just do nothing.

That doesn't fix the problem of a dead label disrupting CSE, though.  Hmm.

I gave up on thinking of a neat, general solution and ended up filing bug 600489 which implements a simple TM-only solution.

Comment 6

[Edwin Smith]

Understandable.  In TR we have a CodegenLabel class that is created at or before the first branch, and remembers patches that need branching.  Later it has enough information to patch the incoming branches and/or not emit the label if there aren't incoming branches.  (but we don't do that optimization because we conservatively assume there could be backedge predecessors we haven't seen yet).  

CodegenLabel has some TR specific things in it, but if you think it would be useful to put something like that in nanojit, I'd be up for it.

Comment 7

[Rick Reitmaier]

What was the issue with the original proposal of using LIR_nop?  

We'd be able to get rid of all the null checks and the code involving branch logic would be roughly the same (replace null check for nop check).

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to comment #7)
> What was the issue with the original proposal of using LIR_nop?  
> 
> We'd be able to get rid of all the null checks and the code involving branch
> logic would be roughly the same (replace null check for nop check).

You mean a completely vanilla LIR_nop, ie. one that you can't call setTarget() on?  It would effectively be the same as a NULL.  Are there places where we have to check for NULL other than before a call to setTarget()?  If so, then a vanilla LIR_nop would be useful.

In other words, what constitutes "all the null checks"?

Comment 9

[Rick Reitmaier]

(In reply to comment #8)
> (In reply to comment #7)
> Are there places where we
> have to check for NULL other than before a call to setTarget()?  If so, then a
> vanilla LIR_nop would be useful.
>

Quite probably but I haven't looked beyond the branching case.  

The one downside to introducing an explicit nop is that storage is required for it.   So, as a general mechanism for denoting an empty instruction its costly as compared to a null pointer.

Comment 10

[Nicholas Nethercote [inactive]]

For future reference:  I just realized we already have a nop of sorts -- LIR_skip.  Furthermore, it can effectively be any size greater than or equal to two words.  There's no way currently to deliberately insert a LIR_skip, but it could be added easily if it was useful.

Comment 11

[Nicholas Nethercote [inactive]]

We also now have LIR_comment, which is a no-op.

Comment 12

[Nicholas Nethercote [inactive]]

(In reply to comment #1)
> 
> If the code calling insGuard() has a similar problem, would it want a LIR_xn
> (exit never) with an intact guard record?  not sure how far we want to carry
> this, given Gal's desire to never emit such a thing.

'exit never' isn't so bad, it's 'exit always' that we really want to avoid.  LIR_xn might be required to fix bug 607856.

Comment 13

[Rick Reitmaier]

(In reply to comment #11)
> re: LIR_skip and LIR_comment

Good to point out ops that also result in no code being generated, but I'd recommend that we don't want to clutter the semantics of any of them by overloading.

Not to imply that you're proposing any such change, but just to point out.

Comment 14

[Nicholas Nethercote [inactive]]

Nanojit has been dead for several years. Its Bugzilla component has been moved to the graveyard (bug 984276).

I checked all the open bugs. They're all uninteresting, so I'm WONTFIXing them all. Apologies for the bugspam.