IM+TI: check for int32 when loading doubles from property/array slots

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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++)
        print(f());
    a = 20;
    print(f());

}
test();
--
With --ion -n, this prints 20 instead of 20.2
(Assignee)

Comment 1

6 years ago
Created attachment 577680 [details] [diff] [review]
Fix

- 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.
Attachment #577680 - Flags: review?(dvander)
Comment on attachment 577680 [details] [diff] [review]
Fix

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.
Attachment #577680 - Flags: review?(dvander)
(Assignee)

Comment 3

6 years ago
Created attachment 578650 [details] [diff] [review]
Patch v2

Adds an out-of-line path to MUnbox.
Attachment #577680 - Attachment is obsolete: true
Attachment #578650 - Flags: review?(dvander)
(Assignee)

Comment 4

6 years ago
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.
Attachment #578650 - Attachment is obsolete: true
Attachment #578650 - Flags: review?(dvander)
Attachment #578820 - Flags: review?(dvander)
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?
Attachment #578820 - Flags: review?(dvander) → review+
(Assignee)

Comment 6

6 years ago
Created attachment 580032 [details] [diff] [review]
Patch

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.
Attachment #578820 - Attachment is obsolete: true
Attachment #580032 - Flags: review?(dvander)
(Assignee)

Comment 7

6 years ago
Created attachment 580047 [details] [diff] [review]
Patch

Now also moves entry-binding to the OOL visitor.
Attachment #580032 - Attachment is obsolete: true
Attachment #580032 - Flags: review?(dvander)
Attachment #580047 - Flags: review?(dvander)
Comment on attachment 580047 [details] [diff] [review]
Patch

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.
Attachment #580047 - Flags: review?(dvander) → review+
(Assignee)

Comment 9

6 years ago
Pushed with OutOfLineUnboxDouble moved to CodeGenerator.cpp:

http://hg.mozilla.org/projects/ionmonkey/rev/351e94bf59cb
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.