Closed
Bug 1135038
Opened 8 years ago
Closed 8 years ago
SIMD: Implement MSimdCheck
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: bbouvier, Assigned: ivan)
References
Details
Attachments
(2 files, 2 obsolete files)
12.00 KB,
patch
|
Details | Diff | Splinter Review | |
6.64 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
So in Odin, SIMD.type.check is a no-op, as it just validates a type that we've already been able to pre-determine. In Ion, the situation is different: either we can eliminate the check during GVN, or we have to implement the check in assembly and maybe throw there.
Reporter | ||
Comment 1•8 years ago
|
||
Assigning to Ivan, as discussed on irc :) Thanks for your help here!
Assignee: nobody → ivan
Assignee | ||
Comment 2•8 years ago
|
||
WIP patch, it builds with current inbound. Search for TODO for missing parts
Attachment #8569068 -
Flags: feedback?(benj)
Reporter | ||
Comment 3•8 years ago
|
||
Comment on attachment 8569068 [details] [diff] [review] WIP. Starting point for MSimdCheck Review of attachment 8569068 [details] [diff] [review]: ----------------------------------------------------------------- While I was looking at this patch, I realized one simplification: if the MSimdCheck has a SimdPolicy as a type policy, we don't even need to handle the case for bailouts, as the bailout will be done by the SimdUnbox placed right before the instruction, making the instruction a no-op. But, in that case, why would we just have a MIRNode for this? I've experimented, and we can just inline a sequence of SimdUnbox / SimdBox as a call to .check, that's pretty nice. I will post my patch afterwards. Sorry for the mess here, as usual implementation details and tricks came to mind during implementation. Ivan, I feel very sorry for all the work done here and that gets thrown away. Would you be interested in working on another SIMD bug which doesn't have such tricks? For instance, bug 1135040 will need to implement new codegen instructions only, iirc (psrad with variable shift count and so on).
Attachment #8569068 -
Flags: feedback?(benj)
Reporter | ||
Comment 4•8 years ago
|
||
As stated on irc, let's just do a SimdUnbox/SimdBox sequence, with a guard on the SimdUnbox so that it doesn't get erased as unused code.
Attachment #8569149 -
Flags: review?(nicolas.b.pierron)
Reporter | ||
Comment 5•8 years ago
|
||
Better testing.
Attachment #8569149 -
Attachment is obsolete: true
Attachment #8569149 -
Flags: review?(nicolas.b.pierron)
Attachment #8569154 -
Flags: review?(nicolas.b.pierron)
Reporter | ||
Comment 6•8 years ago
|
||
Now with the correct test file.
Attachment #8569154 -
Attachment is obsolete: true
Attachment #8569154 -
Flags: review?(nicolas.b.pierron)
Attachment #8569805 -
Flags: review?(nicolas.b.pierron)
Comment 7•8 years ago
|
||
Comment on attachment 8569805 [details] [diff] [review] check.patch Review of attachment 8569805 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MCallOptimize.cpp @@ +3092,5 @@ > + MSimdUnbox *unbox = MSimdUnbox::New(alloc(), callInfo.getArg(0), mirType); > + // Make sure not to remove this unbox! > + unbox->setGuard(); > + > + return boxSimd(callInfo, unbox, templateObj); note: Another option would have been to push the original object again. This would avoid adding a new MSimdBox, and other MSimdUnbox could be folded with this guard instruction.
Attachment #8569805 -
Flags: review?(nicolas.b.pierron) → review+
Reporter | ||
Comment 8•8 years ago
|
||
Even better! I've used it this way and it works flawlessly. https://hg.mozilla.org/integration/mozilla-inbound/rev/c4ec1a14df2d
https://hg.mozilla.org/mozilla-central/rev/c4ec1a14df2d
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•