Closed Bug 602397 Opened 9 years ago Closed 9 years ago

TM: clean up TraceRecorder::alu()

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

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.
Blocks: 602695
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
Whiteboard: fixed-in-tracemonkey
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.
Whiteboard: fixed-in-tracemonkey
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 :(
http://hg.mozilla.org/tracemonkey/rev/cfff9765e524
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/cfff9765e524
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.