Created attachment 397814 [details] [diff] [review] 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 on attachment 397814 [details] [diff] [review] patch Cool.
I wonder whether its worth grouping the calls such that we can mask-match them. Just an idea.
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.
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.
Hmm, I'll believe it's faster when I see the numbers...
Yeah, its only worth taking that ugly hack if it is actually measurably faster.
Backed out due to Tinderbox crashes: http://hg.mozilla.org/tracemonkey/rev/0fc46fb6cba1
Profiling under 20x-over-compilation (see bug 504258) has shown a very slight slowdown. If bug 515305 is completed that will supersede this anyway.
Created attachment 411614 [details] [diff] [review] 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!
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.
Ed, I'll change the skip test to a while loop that ignores multiple skips in a row.
For the record, this introduced an intermittent bug :( It was fixed in bug 530713.