Closed Bug 1135038 Opened 5 years ago Closed 5 years ago

SIMD: Implement MSimdCheck

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: bbouvier, Assigned: ivan)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Assigning to Ivan, as discussed on irc :) Thanks for your help here!
Assignee: nobody → ivan
WIP patch, it builds with current inbound. Search for TODO for missing parts
Attachment #8569068 - Flags: feedback?(benj)
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)
Attached patch check.patch (obsolete) — Splinter Review
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)
Attached patch check.patch (obsolete) — Splinter Review
Better testing.
Attachment #8569149 - Attachment is obsolete: true
Attachment #8569149 - Flags: review?(nicolas.b.pierron)
Attachment #8569154 - Flags: review?(nicolas.b.pierron)
Attached patch check.patchSplinter Review
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 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+
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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.