Last Comment Bug 705351 - IM+TI: check for int32 when loading doubles from property/array slots
: IM+TI: check for int32 when loading doubles from property/array slots
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: IM+TI
  Show dependency treegraph
Reported: 2011-11-25 12:29 PST by Jan de Mooij [:jandem]
Modified: 2011-12-09 01:11 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Fix (13.63 KB, patch)
2011-11-29 11:14 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch v2 (18.46 KB, patch)
2011-12-02 11:03 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch v3 (23.87 KB, patch)
2011-12-03 02:55 PST, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Splinter Review
Patch (25.82 KB, patch)
2011-12-08 07:02 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch (26.78 KB, patch)
2011-12-08 08:10 PST, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Splinter Review

Description User image Jan de Mooij [:jandem] 2011-11-25 12:29:32 PST
When JM loads a double from a property or element slot, it checks whether it's a double or an int. If it's an int, it's converted to a double. Getelem/getgname in Ion currently don't do this check and this causes correctness issues in some sunspider tests.

Here's an example:
var a = 1.1;
function f(arr) {
    return a + 0.2;
function test() {
    for (var i=0; i<100; i++)
    a = 20;

With --ion -n, this prints 20 instead of 20.2
Comment 1 User image Jan de Mooij [:jandem] 2011-11-29 11:14:21 PST
Created attachment 577680 [details] [diff] [review]

- Adds loadInt32OrDouble, it's used by the LLoad*T instructions. It checks whether the value is an int32 and then either loads as double or converts it to double.

- In pushTypeBarrier, use MToDouble instead of MUnbox for doubles. This ensures we don't bailout when the value is an integer. This handles the LLoad*V instructions. Note that LValueToDouble is not very efficient right now, but it could take a TypeSet* in the future and only emit the int32/double paths.

- Fixes a small bug in pushTypeBarrier (related to null/undefined) and added a test for it.
Comment 2 User image David Anderson [:dvander] 2011-12-01 11:58:13 PST
Comment on attachment 577680 [details] [diff] [review]

Review of attachment 577680 [details] [diff] [review]:

::: js/src/ion/IonBuilder.cpp
@@ +2515,5 @@
> +        break;
> +
> +      case JSVAL_TYPE_DOUBLE:
> +        // Use MToDouble to handle Int32Value -> double too.
> +        barrier = MToDouble::New(ins);

This will generate LValueToDouble which could incorrectly coerce something like "null" to 0.0. Instead, I would make MUnbox have an out-of-line path for int32->double conversion. The MToDouble earlier is okay since it's infallible, but it would be better to use MUnbox there as well to generate less code.
Comment 3 User image Jan de Mooij [:jandem] 2011-12-02 11:03:13 PST
Created attachment 578650 [details] [diff] [review]
Patch v2

Adds an out-of-line path to MUnbox.
Comment 4 User image Jan de Mooij [:jandem] 2011-12-03 02:55:02 PST
Created attachment 578820 [details] [diff] [review]
Patch v3

Moves the bailout to the OOL path. One problem was that masm.framePushed() was always 0 for out-of-line code (since it's generated after the function epilogue) and bailout() expects it to be valid. I think two OOL paths can have different framePushed values so I decided to store the framePushed value in the OOL class and restore it when generating code for the OOL path.
Comment 5 User image David Anderson [:dvander] 2011-12-07 18:13:08 PST
Comment on attachment 578820 [details] [diff] [review]
Patch v3

Review of attachment 578820 [details] [diff] [review]:

Thanks for doing the out-of-line variant. Make sure to have a follow-up patch for ARM - if OutOfLineUnboxDouble could even be shared across all platforms (at the very least, its header), even better.

::: js/src/ion/x86/CodeGenerator-x86.cpp
@@ +250,5 @@
> +{
> +    LUnboxDouble *ins = ool->unboxDouble();
> +    const ValueOperand value = ToValue(ins, LUnboxDouble::Input);
> +
> +    masm.bind(ool->entry());

Sort of unrelated, is it possible to hoist entry-binding to the OOL visitor?
Comment 6 User image Jan de Mooij [:jandem] 2011-12-08 07:02:11 PST
Created attachment 580032 [details] [diff] [review]

If we add LUnboxDouble for x64, we can generate code for it in CodeGenerator.cpp and get rid of the 3 platform-dependent implementations. This also makes it easier to add the OOL path to CodeGenerator.cpp

Asking re-review mostly for the LUnboxDouble on x64.
Comment 7 User image Jan de Mooij [:jandem] 2011-12-08 08:10:23 PST
Created attachment 580047 [details] [diff] [review]

Now also moves entry-binding to the OOL visitor.
Comment 8 User image David Anderson [:dvander] 2011-12-08 18:55:30 PST
Comment on attachment 580047 [details] [diff] [review]

Review of attachment 580047 [details] [diff] [review]:

::: js/src/ion/CodeGenerator.h
@@ +88,5 @@
> +    virtual bool visitOutOfLineUnboxDouble(OutOfLineUnboxDouble *ool);
> +};
> +
> +// An out-of-line path to convert a boxed int32 to a double.
> +class OutOfLineUnboxDouble : public OutOfLineCodeBase<CodeGenerator>

Nit: You could move this into CodeGenerator.cpp to avoid .h clutter.
Comment 9 User image Jan de Mooij [:jandem] 2011-12-09 01:11:22 PST
Pushed with OutOfLineUnboxDouble moved to CodeGenerator.cpp:

Note You need to log in before you can comment on or make changes to this bug.