503990 - nanojit: make isStmt() table-driven

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

isStmt() is currently a rat's nest of conditionals.  This patch makes it table-driven by adding an extra 'isStmt' field to every opcode in LIRopcode.tbl.  It also cleans up the formatting in LIRopcode.tbl a little.

I'm seeing a 3--5ms speedup for SunSpider on my Mac with this change.  In an earlier version I wasn't seeing any speed-up, but I suspect this change interacts well with the change from bug 496693.

Comment 1

[Nicholas Nethercote [inactive]]

Nb: for the review, I'd appreciate it if you could check the isStmt field for
all the opcodes extra carefully -- I'm pretty confident I preserved the meaning
of isStmt(), but it deserves a very close look.

Comment 2

[Andreas Gal :gal]

Comment on attachment 388378 [details] [diff] [review]
patch

Optional nit: The naming is a bit inconsistent (isStmtArray vs repKind vs insSize).

3-5ms speedup is awesome.

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to comment #2)
> (From update of attachment 388378 [details] [diff] [review])
> Optional nit: The naming is a bit inconsistent (isStmtArray vs repKind vs
> insSize).

I can't call it isStmt because there's a function of the same name.  I'll add a comment to that effect.

Comment 4

[Nicholas Nethercote [inactive]]

Sigh... after updating to r30233, I get a 2--6ms slowdown.  I think this is just noise, that really there's no performance effect, but it's worth doing for the code cleanup.

Comment 5

[Nicholas Nethercote [inactive]]

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

Comment 6

[Robert Sayre]

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

Comment 7

[Andreas Gal :gal]

patch asserts in debug mode

-OPDEF64(ldq,    LIR_ld, 1, Ld)      // 64-bit (quad) load
+OPDEF64(ldq, LIR_ld, 2, Ld,  0) // 64-bit (quad) load

operand count changed, hitting an assert

this was just merged and will burn any debug tinder boxes

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: patch that should fix breakage

This should fix the breakage.  I'm not seeing the failure in the shell, though, which is really weird.  I haven't tested the fix on a browser yet, I will shortly.

Comment 9

[Nicholas Nethercote [inactive]]

Ah, I understand now:  it's only occurring for ldc/ldcs/ldcb, which don't get used in trace-tests.js.  The operand count is also wrong for ld/ldq/ldqc, but the operand count is never used for those ones.  (One of the reasons I don't like the operand count much is that it's never used for many of the opcodes and so many of them could be wrong without consequence.)

Comment 10

[Andreas Gal :gal]

Comment on attachment 388412 [details] [diff] [review]
patch that should fix breakage

I was about to upload the same patch, except that my browser keeps GCing. Another patch is bad too.

Comment 11

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/7fc9bac4d871

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Is it worth it to add testcases to trace-test.js that exercise those opcodes?

Comment 13

[Robert Sayre]

had to back this out for the time being, to sort out some regressions. it can probably go back in shortly.

Comment 14

[Nicholas Nethercote [inactive]]

(In reply to comment #12)
> Is it worth it to add testcases to trace-test.js that exercise those opcodes?

Definitely.  But I don't know what those test cases are.  Looking at the code they seem to be generated in jsregexp.cpp.

Comment 15

[Nicholas Nethercote [inactive]]

This will conflict horribly with bug 504506, and that one is more important, so I'm marking this as dependent.

Comment 16

[Nicholas Nethercote [inactive]]

The patch is overly conservative for calls -- it assumes all calls are stmts, when some of them (the CSEable ones) are not.  Hmm.

Comment 17

[Nicholas Nethercote [inactive]]

This can be done (mostly) via the retType field added by the patch in bug 504507 -- all statements have void return types except for some calls.  Preliminary tests indicate a small (eg. 3ms) SunSpider speedup with this change.

Also removing the dependency on bug 504506, this one could be done first.

Comment 18

[Nicholas Nethercote [inactive]]

Attachment: patch v3

This patch changes isStmt() to use retType[] for all cases except calls.
Performance changes are in the noise for SunSpider, but it's fewer
instructions, fewer branches, and neater code, which adds up to a win in
my books.

One side-effect of this is that LIR_file and LIR_line are now considered
statements, they previously weren't.  I think the old behaviour was a bug,
because it meant LIR_file/LIR_line never did anything -- I think those
opcodes are broken, they only do something if VTUNE is defined anyway.

Comment 19

[Rick Reitmaier]

Comment on attachment 419051 [details] [diff] [review]
patch v3

Seems fine and yes I think this fixes a LIR_line and LIR_file bug, they should be treated as statements.

Comment 20

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/projects/nanojit-central/rev/4d331f0d6a04

Comment 21

[Nicholas Nethercote [inactive]]

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

Comment 22

[Robert Sayre]

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