1135038 - SIMD: Implement MSimdCheck

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier]]

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.

Comment 1

[Benjamin Bouvier [:bbouvier]]

Assigning to Ivan, as discussed on irc :) Thanks for your help here!

Comment 2

[Ivan Jibaja]

Attachment: WIP. Starting point for MSimdCheck

WIP patch, it builds with current inbound. Search for TODO for missing parts

Comment 3

[Benjamin Bouvier [:bbouvier]]

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).

Comment 4

[Benjamin Bouvier [:bbouvier]]

Attachment: check.patch

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.

Comment 5

[Benjamin Bouvier [:bbouvier]]

Attachment: check.patch

Better testing.

Comment 6

[Benjamin Bouvier [:bbouvier]]

Attachment: check.patch

Now with the correct test file.

Comment 7

[Nicolas B. Pierron [:nbp]]

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.

Comment 8

[Benjamin Bouvier [:bbouvier]]

Even better! I've used it this way and it works flawlessly.

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

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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