513865 - nanojit: make LirReader::read() clearer and faster

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

With LIR_2 gone, LirReader::read() can be reworked to be clearer and faster.
Specifically:

- Currently the loop never has a skip on the first iteration, but only has
  skips on the 2nd and subsequent iterations.  So I split it into a single
  switch (which doesn't have to handle skip) followed by a skip-chasing
  loop.  This makes things clearer.

- The LIR_start case can be skipped in optimised builds, avoiding one
  comparison.  Doesn't make a noticeable difference to SunSpider, but it
  can't hurt in the long run.

- Added some comments making some invariants clearer.

- Removed setpos(), which is dead.

Comment 1

[Andreas Gal :gal]

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

Cool.

Comment 2

[Andreas Gal :gal]

I wonder whether its worth grouping the calls such that we can mask-match them. Just an idea.

Comment 3

[Nicholas Nethercote [inactive]]

I don't understand -- what do you mean by "mask-match"?

It's a shame that calls are variable-length.  I wonder if making them fixed length -- unused arguments would just be wasted space -- would be worth it just to avoid this switch.

Comment 4

[Andreas Gal :gal]

icall is 16. fcall is 16 | 64 and qcall is 17 | 64. So

((iop &~ (64 | 1)) == 16)

should be equivalent. This would need some asserts and explanation in LIRopcode.tbl and blocking of 17 (which is currently unused anyway).

Its a bit ugly, but might be fast.

Comment 5

[Nicholas Nethercote [inactive]]

Hmm, I'll believe it's faster when I see the numbers...

Comment 6

[Andreas Gal :gal]

Yeah, its only worth taking that ugly hack if it is actually measurably faster.

Comment 7

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/48928150aa27

Comment 8

[Nicholas Nethercote [inactive]]

Backed out due to Tinderbox crashes:

http://hg.mozilla.org/tracemonkey/rev/0fc46fb6cba1

Comment 9

[Nicholas Nethercote [inactive]]

Profiling under 20x-over-compilation (see bug 504258) has shown a very slight slowdown.  If bug 515305 is completed that will supersede this anyway.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/48928150aa27

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/0fc46fb6cba1 -- oops.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: new patch

Thanks to bug 525437, which removed the special-casing required for calls, this is back on the table.  For an optimised build we now just (a) do a table-lookup to get the instruction size, step back that many bytes, and (b) check if we're now on a skip, and if so follow it back to the previous chunk.  No switches required!

Comment 13

[Edwin Smith]

tamarin has code that erases instructions by turning them into a LIR_skip, but that could cause read() to see two skips in a row.  so, maybe don't want that assert there at least until tamarin's erasure code is changed.

Comment 14

[Nicholas Nethercote [inactive]]

Ed, I'll change the skip test to a while loop that ignores multiple skips in a row.

Comment 15

[Nicholas Nethercote [inactive]]

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

Comment 16

[Nicholas Nethercote [inactive]]

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

Comment 17

[Robert Sayre]

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

Comment 18

[Nicholas Nethercote [inactive]]

For the record, this introduced an intermittent bug :(  It was fixed in bug 530713.