Closed
Bug 1078696
Opened 10 years ago
Closed 10 years ago
MObjectState / MArrayState should be transparent for Float32 type.
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: nbp, Assigned: nbp)
References
Details
Attachments
(1 file, 1 obsolete file)
8.30 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
Recovered on Bailout instruction which are produced before the Apply Type phase should be transparent for the Float32 coercion.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8500645 -
Flags: review?(benj)
Comment 2•10 years ago
|
||
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+
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8504111 -
Flags: review?(benj)
Comment 4•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Attachment #8500645 -
Attachment is obsolete: true
Assignee | ||
Comment 5•10 years ago
|
||
Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ea34a3e44c20 https://hg.mozilla.org/integration/mozilla-inbound/rev/f4be6b8ddbe2
Comment 6•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f4be6b8ddbe2
Assignee: nobody → nicolas.b.pierron
Status: NEW → RESOLVED
Closed: 10 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.
Description
•