Status

Core Graveyard
Nanojit
P2
normal
RESOLVED WONTFIX
8 years ago
4 years ago

People

(Reporter: njn, Unassigned)

Tracking

unspecified
flash10.1.x-Salt
Bug Flags:
flashplayer-qrb +

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

8 years ago
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

8 years ago
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.

Updated

8 years ago
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Future
(Reporter)

Updated

8 years ago
Blocks: 600127
(Reporter)

Comment 2

8 years ago
(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.
(Reporter)

Comment 3

8 years ago
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?
(Reporter)

Comment 4

8 years ago
(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.
(Reporter)

Comment 5

8 years ago
(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.
No longer blocks: 600127

Comment 6

8 years ago
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

8 years ago
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).
(Reporter)

Comment 8

8 years ago
(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"?

Updated

8 years ago
Flags: flashplayer-qrb+
Priority: -- → P2
Target Milestone: Future → flash10.1.x-Salt

Comment 9

8 years ago
(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.
(Reporter)

Comment 10

8 years ago
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.
(Reporter)

Comment 11

8 years ago
We also now have LIR_comment, which is a no-op.
(Reporter)

Comment 12

8 years ago
(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

8 years ago
(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.
(Reporter)

Updated

7 years ago
Assignee: nnethercote → nobody
Status: ASSIGNED → NEW
(Assignee)

Updated

4 years ago
Component: Nanojit → Nanojit
Product: Core → Core Graveyard
(Reporter)

Comment 14

4 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.