602397 - TM: clean up TraceRecorder::alu()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

On a related note, TraceRecorder::unary() only ever takes LIR_noti as the 'op' argument, and so it could be simplified.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: patch (against TM 63014:513774b1e88e)

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.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 4

[Nicholas Nethercote [inactive]]

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.

Comment 5

[Nicholas Nethercote [inactive]]

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

Comment 6

[Phil Ringnalda (:philor)]

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.

Comment 7

[Nicholas Nethercote [inactive]]

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.

Comment 8

[Nicholas Nethercote [inactive]]

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 :(

Comment 9

[Nicholas Nethercote [inactive]]

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

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

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