495158 - nanojit: fix incorrect operand counts in LIRopcode.tbl?

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch altering LIRopcode.tbl operand counts

LIRopcode.tbl has an operand count for every LIR instruction.  Some of these look to be wrong;  at least, they don't match the number of operands as determined by the particular insertion function (eg. ins1(), ins2()) used to create them.

The attached patch is an attempt to fix the ones I could see didn't match.  I had to disable an assertion for the change to LIR_xt/LIR_xf to be acceptable.  I'm not sure if the patch is correct because I'm not sure what the exact definition of "operand" used in LIRopcode.tbl is.  And the fact that the assertion in CseFilter::insGuard is conditional on isCseOpcode() seems strange.

I also don't know how the operand counts are used... eg. I tried changing the operand count for LIR_add to 1 and, after disabling one failing assertion, trace-test.js ran fine.  So that concerns me a bit.

So if my patch is incorrect, I guess I'd like to see a comment in LIRopcode.tbl explaining exactly what the operand count represents and what the effects are if it is wrong.

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 380015 [details] [diff] [review]
patch altering LIRopcode.tbl operand counts

I don't know what the operand counts mean, unfortunately.  Prior to the addition of that file those values were in a huge array, separate from a number of other arrays for per-opcode information, and I just blindly copied them over.  Ed or any other nanojit hacker certainly knows better than I do, in any case.

Comment 2

[Andreas Gal :gal]

Comment on attachment 380015 [details] [diff] [review]
patch altering LIRopcode.tbl operand counts

THe assert in insGuard is correct in the sense that if we do find1, we better only stick unary lir instructions in there. The code around it might be buggy of course.

I am not sure loop really has 2 args. x might have a constant as 1st arg always (have to check). This might need some cleanup outside the table as well.

I am pretty sure xbarrier is unary. And xtbl too.

A lot of this points to inconsistencies and potential bugs, but the patch isn't enough to fix it.

Comment 3

[Nicholas Nethercote [inactive]]

I looked at tamarin-redux.  The values for file, line, xbarrier and xtbl match
those in my patch.  The values for the others match what is currently in TM. 
Confusing.

In response to comment #2, I didn't really expect it to fix the problems, rather to highlight them.  If we don't know how many operands all our instructions have... that's a problem.

Comment 4

[Edwin Smith]

(In reply to comment #3)
> In response to comment #2, I didn't really expect it to fix the problems,
> rather to highlight them.  If we don't know how many operands all our
> instructions have... that's a problem.

especially with a variable-sized LIns!

Comment 5

[Nicholas Nethercote [inactive]]

Working on bug 498807, I've come to understand this better.  It's confusing because the number of what-are-considered-operands (aka. the arity) doesn't necessarily match the number of fields needed for the instruction.  To clarify:

xt/xf: have two fields, the condition, and the GuardRecord.  But the GuardRecord isn't considered an operand.  So arity=1

loop/x/xbarrier: have two fields, the condition, and the GuardRecord.  But the condition is always set to insImm(1) and subsequently ignored.  So arity=0.  But xbarrier is listed as arity=1 in LIRopcode.tbl, which I'm pretty sure should be 0.

xtbl: has two fields, the difference value (whatever that is), and the GuardRecord.  So arity=1.

jf/jt: have two fields, the condition, and the target label.  The target label isn't considered an operand.  So arity=1.  (Nb: All the calls to insBranch() that create jf/jt instructions, except those in lirasm.cpp, set the target to NULL... I haven't worked out why that is yet.  Do they get patched later?  If so, that argument could be removed from insBranch().)

Hopefully this clears things up (Graydon, Julian and I had a very confused discussion about this on IRC recently).  My in-progress patch for bug 492866 will make the number of fields needed for each opcode clearer, and I have some ideas for bug 498807 that will clarify the confusion between the different kinds of guards (conditional, unconditional, and switch).  I'll come back to this once they're done.

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to comment #5)
> 
> xt/xf: have two fields, the condition, and the GuardRecord.  But the
> GuardRecord isn't considered an operand.  So arity=1

Thinking about this more, I'm concerned this could cause problems with CseFilter.

CseFilter only looks at the "operand", ie. the condition, when deciding if two LIR_xt instructions are equivalent (and likewise for LIR_xf).  AFAICT, if two LIR_xt/xf instructions had identical opcodes and conditions but different GuardRecords they will be incorrectly commoned up.

Can anyone see why the above reasoning might be wrong?

Comment 7

[Nicholas Nethercote [inactive]]

> (In reply to comment #5)
> But xbarrier is listed as arity=1 in LIRopcode.tbl, which I'm pretty sure
> should be 0.

The commit in bug 504705 fixes this.  So the only issue left to be resolved in this bug is the one in comment 6.  (Well, ultimately I want to kill of the operandCount field altogether, but that's another bug...)

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to comment #7)
> So the only issue left to be resolved in
> this bug is the one in comment 6.

Actually, LIR_file and LIR_line should probably have operandCount of 1.  But bug 505662 will remove operandCount altogether.  So comment 6 is the only thing left to worry about here.

Comment 9

[Edwin Smith]

(In reply to comment #6)
> (In reply to comment #5)
> > 
> > xt/xf: have two fields, the condition, and the GuardRecord.  But the
> > GuardRecord isn't considered an operand.  So arity=1
> 
> Thinking about this more, I'm concerned this could cause problems with
> CseFilter.
> 
> CseFilter only looks at the "operand", ie. the condition, when deciding if two
> LIR_xt instructions are equivalent (and likewise for LIR_xf).  AFAICT, if two
> LIR_xt/xf instructions had identical opcodes and conditions but different
> GuardRecords they will be incorrectly commoned up.
> 
> Can anyone see why the above reasoning might be wrong?

If the first guard is taken (exits), the second is never reached (*).  if the first guard is not taken, then neither is the second one, so it shouldn't matter if the second one has a different guard record.

(*) assuming there's never a path from the side exit of guard 1, back to guard 2.  for tree-shaped fragments this should be true.  Also, this could be an invalid argument if there is important information in the guard record *other* than just whats needed to execute a successful bailout.  but i can't think of anything like that.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: patch describing CSE-of-guard assumptions

Ah, I see.  It also assumes that the CSE algorithm always preserves the first guard seen and removes the second one -- that is true for the current code but not true for some conceivable alternatives.

I've attached a patch that adds a comment describing all this.

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: reworded the comment

Comment 12

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/c580e9a2a18e

Comment 13

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/c580e9a2a18e