Closed Bug 602397 Opened 9 years ago Closed 9 years ago
TM: clean up Trace
TraceRecorder::alu() is a big horrible function, with confusing control flow. It should be cleaned up, probably by breaking it into pieces. Attachment 457718 [details] [diff] from bug 505876 makes a start on this.
On a related note, TraceRecorder::unary() only ever takes LIR_noti as the 'op' argument, and so it could be simplified.
This patch rewrites alu(): - alu() is called for integer ops and double ops. But for integer ops it just inserts them as-is. So I changed it to alu_double() and made the integer op cases do the insertion directly. - Then I simplified the control flow greatly by having a single switch in alu_double() with one case per opcode, instead of two switches with some code between them and some awkward sharing of cases between opcodes. This allowed ChecksRequired() and guard_xov() to be inlined. The end result is a small amount of duplication between some of the cases (most notably between LIR_addd and LIR_subd) but the patch overall is a 43 line code reduction and the control flow is so much simpler -- a single swich, with one fall-back goto path for undemotable operations -- that it's totally worthwhile. - It also rewrites unary() to take advantage of the fact that it only takes integer ops (actually, it only takes LIR_not). I checked that the LIR produced for Sunspider is unchanged by this patch.
Attachment #517332 - Flags: review?(wmccloskey)
Comment on attachment 517332 [details] [diff] [review] patch (against TM 63014:513774b1e88e) This is a really nice simplification. Could you also check that it generates the same results for v8-crypto? That was the one that I remember being really delicate. Also, alu_double might not be the best name. Maybe something like tryWithIntMath?
Attachment #517332 - Flags: review?(wmccloskey) → review+
I've found V8 LIR codegen to be non-deterministic, so I generally just use Sunspider which is deterministic except for 3d-raytrace and occasionally access-nbody. But I'll take a look at v8-crypto and see if it looks reasonable.
v8-crypto was too non-deterministic for me to check it meaningfully. And I ended up rename alu_double() as tryToDemote(). http://hg.mozilla.org/tracemonkey/rev/cc7311c09b56
Though it wasn't immediately obvious (since it skipped running on your push, and mobile Talos dies all the time anyway), this broke tp4 and tsspider on Maemo - they either die with an unhelpful "browser non-zero return code (35584)" or just hang. I pushed a backout to the tryserver, and it was green.
Backed out: http://hg.mozilla.org/tracemonkey/rev/2b4db9e5e3eb This was meant to be a pure refactoring patch, ie. no functional change, but I must have screwed it up. Sorry for the breakage.
Ah, I had the LIR_divd/LIR_modd cases inside a "#ifdef x86" block. But they can occur on ARM, they just can't be demoted. So I added those cases for ARM, doing no demotion in them, and got green on the try server. If ARM tests were run on debug builds I would have got an assertion failure, which would have made things much more obvious :(
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.