1078696 - MObjectState / MArrayState should be transparent for Float32 type.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Recovered on Bailout instruction which are produced before the Apply Type phase should be transparent for the Float32 coercion.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Recovered objects and array should not prevent Float32 operations.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8500645 [details] [diff] [review]
Recovered objects and array should not prevent Float32 operations.

Review of attachment 8500645 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, can you please file a follow up or produce a new patch in this bug for the remark below?

::: js/src/jit-test/tests/ion/bug1078696.js
@@ +1,1 @@
> +setJitCompilerOption("ion.warmup.trigger", 30);

can you please give a better name to this test case, like test-scalar-replacement-float32.js or test-object-array-states-float32.js please? (the important part being that float32 is in the title, so we can filter by the string "float32".)

@@ +5,5 @@
> +        a: Math.fround(x + 1),
> +        b: Math.fround(x - 1)
> +    };
> +
> +    return tmp.a + tmp.b;

I'd prefer to have some assertFloat32 here (and below). However, if I understand correctly, this addition can be carried out with float32 specialization with scalar replacement (well, if we wrap the result in a Math.fround() call), and can't without it, so that would make the test dependent on whether SR is activated or not. Could you file a follow-up to add scalar replacement to the list of JIT compiler options, and modify this test as needed please?

::: js/src/jit/MIR.h
@@ +2633,5 @@
>      }
>  
> +    // As opposed to MStoreSlot / MStoreFixedSlot, an instruction which is
> +    // recovered on bailout does not distinguish between float32 and doubles as
> +    // the SnapshotIterator converts every inputs back to a Double.

This sounds unclear to me. I think this stands true because any instruction specialized as float32 and present in a MObjectState will have to be recovered, and if the specialization matters, a call to RoundFloat32 in the recover instruction will effectively act as a consumer (i.e. do the same as Math.fround()). Could you update this comment?

@@ +2723,5 @@
>      }
>  
> +    // As opposed to the MStoreElement, an instruction which is recovered on
> +    // bailout does not distinguish between float32 and doubles as the
> +    // SnapshotIterator converts every inputs back to a Double.

Might be safer to refer to the comment in MObjectState, modulo MStoreElement instead of MStoreSlot / MStoreFixedSlot.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Recovered store instructions should prevent Float32 operations.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8504111 [details] [diff] [review]
Recovered store instructions should prevent Float32 operations.

Review of attachment 8504111 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, I assume this replaces the previous one.

::: js/src/jit-test/tests/ion/test-scalar-replacement-float32.js
@@ +13,5 @@
> +}
> +
> +var func = null;
> +var check_object_argument_func = function (i, res) {
> +    with ({}) { };

please add a comment that this won't be jitted because of this statment

::: js/src/jit/TypePolicy.cpp
@@ +510,5 @@
> +{
> +    for (size_t op = FirstOp, e = def->numOperands(); op < e; op++)
> +        EnsureOperandNotFloat32(alloc, def, op);
> +    return true;
> +    return true;

nit: YORTO (You Only Return True Once)

Comment 5

[Nicolas B. Pierron [:nbp]]

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ea34a3e44c20

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4be6b8ddbe2

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f4be6b8ddbe2