502788 - nanojit: avoid many redundant NULL comparisons in the LirReader pipeline

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

The termination condition in the LirReader pipeline is when the returned LInsp is NULL.  This means that the first thing every step in the pipeline does is a NULL check.  Lots of redundancy.

The attached patch changes the termination condition to opcode==LIR_start.  This means that the stages in the pipeline no longer need to test against NULL.  It also means the pipeline is one instruction shorter (because you stop on LIR_start, not after it).

It's a clear win in the number of instructions executed, but doesn't make any measurable time difference for SunSpider, probably because the NULL tests are highly predictable.  (It doesn't help that NJ compilation time is maybe 10--15% of overall SunSpider time.) 

I'm wondering what is the fate of such patches -- are they worth committing in the hope that they might make a difference on some other machine, or in the future after some other changes have been made?  Or do they just wither and die?

Comment 1

[Brendan Eich [:brendan]]

The patch reduces code size (compiled and source), right? That alone (no other wins, and assuming no negative effects) makes it a keeper in my book.

/be

Comment 2

[Edwin Smith]

agreed

Comment 3

[Nicholas Nethercote [inactive]]

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

(In reply to comment #1)
> The patch reduces code size (compiled and source), right? That alone (no other
> wins, and assuming no negative effects) makes it a keeper in my book.

It reduces source size.  Binary size turns out the same -- the LIR_start comparisons are a little larger than the NULL comparisons, and the new version has some worse register allocation and so there are some extra movs put in there.  If the pipeline gets longer it should be a clearer win.

Ed, are you willing to officially bless with r+?

Comment 4

[Andreas Gal :gal]

How about a pre-computed table for dce yes/no? Or maybe a precomputed bit-field. Its currently a fairly long chain of short branches (isCse() is internally a bunch of branches too). I made a similar patch a while back after looking at the code in shark.

Comment 5

[Andreas Gal :gal]

(separate bug probably, fly-by r=me for this change, shorter code is always better)

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to comment #4)
> How about a pre-computed table for dce yes/no? Or maybe a precomputed
> bit-field. Its currently a fairly long chain of short branches (isCse() is
> internally a bunch of branches too). I made a similar patch a while back after
> looking at the code in shark.

I tried that, it didn't seem to speed up SS at all.  I might go back and look at it more carefully soon.

Comment 7

[Edwin Smith]

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

We probably need a single table of instruction attributes instead of a bunch of separate tables, (analogous to CallInfo) but this change seems fine.

Comment 8

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/91cb8d578da7

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/91cb8d578da7