Closed Bug 1078696 Opened 5 years ago Closed 5 years ago

MObjectState / MArrayState should be transparent for Float32 type.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file, 1 obsolete file)

Recovered on Bailout instruction which are produced before the Apply Type phase should be transparent for the Float32 coercion.
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.
Attachment #8500645 - Flags: review?(benj) → review+
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)
Attachment #8504111 - Flags: review?(benj) → review+
Attachment #8500645 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/f4be6b8ddbe2
Assignee: nobody → nicolas.b.pierron
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.