User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.95 Safari/537.36

Implement MTruncateToInt32 in js/src/jit/Recover.cpp. See Bug 1003801 comment 0 for explanation.

The implemented recovering code and its test case.

Update the patch. It can pass the try server testing: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b74bb7207092

Comment on attachment 8547286 [details] [diff] [review] Recover_MTruncateToInt32.patch Review of attachment 8547286 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/ion/dce-with-rinstructions.js @@ +1029,5 @@ > } > > +var uceFault_trunc_to_int32_number = eval(uneval(uceFault).replace('uceFault', 'uceFault_trunc_to_int32_number')); > +function rtrunc_to_int32_number(i) { > + var x = Math.imul(i + 0.12, i + 0.23); use a bitwise operator instead of Math.imul, such as (i + 0.12) | 0 @@ +1041,5 @@ > + var t1 = i + 0.12; > + var t2 = i + 0.23; > + var o1 = { valueOf: function() { return t1; } }; > + var o2 = { valueOf: function() { return t2; } }; > + var x = Math.imul(o1, o2); same here. ::: js/src/jit/MIR.h @@ +4638,5 @@ > #endif > > + bool writeRecoverData(CompactBufferWriter &writer) const; > + bool canRecoverOnBailout() const { > + return true; I am surprised the test case does highlight the issue, Can you verify with iongraph that the second test is using recover instructions. ::: js/src/jit/Recover.cpp @@ +1104,5 @@ > +{ > + RootedValue result(cx); > + > + // Directly apply public conversion. > + double in = iter.read().toNumber(); Use: if (!ToNumber(cx, val, &dbl)) return false; instead of toNumber, as the ToNumber function will handle the conversion from string to integers. Add a test case for strings such as assertEq( (i + "0") | 0, 990)

Update the number conversion and refine the test cases.

The iongraph for rtrunct_to_int32_number

The iongraph for rtrunc_to_int32_object

Comment on attachment 8547643 [details] [diff] [review] Recover_MTruncateToInt32.patch Review of attachment 8547643 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/ion/dce-with-rinstructions.js @@ +1031,5 @@ > +var uceFault_trunc_to_int32_number = eval(uneval(uceFault).replace('uceFault', 'uceFault_trunc_to_int32_number')); > +function rtrunc_to_int32_number(i) { > + var x = (i + 0.12) | 0; > + if (uceFault_trunc_to_int32_number(i) || uceFault_trunc_to_int32_number(i)) > + assertEq(x, (i + 0.12) | 0); replace this by 99. @@ +1042,5 @@ > + var o1 = { valueOf: function() { return t1; } }; > + var x = o1 | 0; > + t1 = 777.12; > + if (uceFault_trunc_to_int32_object(i) || uceFault_trunc_to_int32_object(i)) > + assertEq(x, (i + 0.12) | 0); same here. @@ +1049,5 @@ > +var uceFault_trunc_to_int32_string = eval(uneval(uceFault).replace('uceFault', 'uceFault_trunc_to_int32_string')); > +function rtrunc_to_int32_string(i) { > + var x = (i + "0") | 0; > + if (uceFault_trunc_to_int32_string(i) || uceFault_trunc_to_int32_string(i)) > + assertEq(x, (i + "0") | 0); replace it by 990 ::: js/src/jit/MIR.h @@ +4638,5 @@ > #endif > > + bool writeRecoverData(CompactBufferWriter &writer) const; > + bool canRecoverOnBailout() const { > + return true; Can you attach the last MIR phase of the 3 test function that are added in dce-with-rinstructions.js ? I really doubt that we want to return true here.

The iongraph for rtrunc_to_int32_string. It is weird... As these graph show, the truncatetoint32 operations are not colored as gray. However, the program will call RTruncateToInt32::recover() when I set a break point at the function prologue for verification.

Add the input MIR type checking for canRecoverOnBailout().

Comment on attachment 8548995 [details] [diff] [review] Recover_TruncateToInt32.patch Review of attachment 8548995 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MIR.h @@ +4638,5 @@ > #endif > > + bool writeRecoverData(CompactBufferWriter &writer) const; > + bool canRecoverOnBailout() const { > + return input()->type() != MIRType_Object; We probably want an inequality, in order to prevent accepting boxed objects.

Refine the return condition for canRecoverOnBailout(). The try testing result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e3e602a7582

The iongraph for rtrunc_to_int32_number.

The iongraph for rtrunc_to_int32_object

The iongraph for rtrunc_to_int32_string

Comment on attachment 8551302 [details] [diff] [review] Recover_TruncateToInt32.patch Review of attachment 8551302 [details] [diff] [review]: ----------------------------------------------------------------- Sounds good to land :)

Hi, this patch failed to apply: applying Recover_TruncateToInt32.patch patching file js/src/jit-test/tests/ion/dce-with-rinstructions.js Hunk #2 FAILED at 1187 1 out of 2 hunks FAILED -- saving rejects to file js/src/jit-test/tests/ion/dce-with-rinstructions.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh Recover_TruncateToInt32.patch could you take a look, thanks!

Resolve the conflict.

Comment on attachment 8553151 [details] [diff] [review] Recover_TruncateToInt32.patch Review of attachment 8553151 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/Recover.cpp @@ +1143,5 @@ > + > + double in; > + if (!ToNumber(cx, value, &in)) > + return false; > + int out = ToInt32(in); int out = JS::ToInt32(in);

Code refinement: Change ToNumber() to ToInt32() for value range testing. Try testing result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=addfb387c306

Code refinement: Change ToNumber() to ToInt32() for value range testing. Try testing result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=addfb387c306

Comment on attachment 8554584 [details] [diff] [review] Recover_TruncateToInt32_Refine.patch Review of attachment 8554584 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/Recover.cpp @@ +1141,5 @@ > RootedValue value(cx, iter.read()); > RootedValue result(cx); > > + int32_t trunc; > + if (!ToInt32(cx, value, &trunc)) Add the JS:: namespace specifier. And make sure jsapi.h is #include in this file.

1. Include the jsapi.h 2. Add the JS:: namespace specification

Comment on attachment 8554605 [details] [diff] [review] Recover_TruncateToInt32_Refine.patch Review of attachment 8554605 [details] [diff] [review]: ----------------------------------------------------------------- This looks good :)

Before check-in, It should be better to run the test again: https:https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e0db6e9b100