1112158 - GVN: Folds MSimdUnbox to MSimdBox operands.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

As we are boxing every simd operation, because of the object identity, and for
simplicity of development, we want to avoid the boxing/unboxing of SIMD values
when we can keep a copy of the values inside SIMD registers.

This would cause issue with spilling, as we might attempt to keep too many
SIMD registers alive.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Optimize MSimdUnbox with GVN.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8566074 [details] [diff] [review]
Optimize MSimdUnbox with GVN.

Review of attachment 8566074 [details] [diff] [review]:
-----------------------------------------------------------------

See comment in MIR.cpp

::: js/src/jit-test/tests/SIMD/complex-4.js
@@ +5,5 @@
> +setJitCompilerOption("ion.warmup.trigger", 90);
> +var max = 100; // Make have the warm-up counter high enough to
> +               // consider inlining functions.
> +
> +var f4 = SIMD.int32x4; // :TODO: Support float32x4 arith.

f4, int32x4? okay well this is a test case :)

@@ +78,5 @@
> +for (var i = 0; i < max; i++) {
> +  var res = test(cardinals);
> +  assertEqX4(c4norm(res), f4.splat(16 * 16));
> +  assertEqX4(res.re, cardinals16.re, assertEqDbl);
> +  assertEqX4(res.im, cardinals16.im, assertEqDbl);

why not using assertEq here?

::: js/src/jit/MIR.cpp
@@ +943,5 @@
> +        if (!phi->reserveLength(boxedPhi->numOperands()))
> +            return nullptr;
> +
> +        for (size_t i = 0, e = boxedPhi->numOperands(); i < e; i++) {
> +            MDefinition *op = boxedPhi->getOperand(i);

Shouldn't you check a few more things:
- op->isSimdBox()
- op->toSimdBox()->input()->type() == type()

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> Comment on attachment 8566074 [details] [diff] [review]
> Optimize MSimdUnbox with GVN.
> 
> Review of attachment 8566074 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> See comment in MIR.cpp
> 
> ::: js/src/jit-test/tests/SIMD/complex-4.js
> @@ +5,5 @@
> > +setJitCompilerOption("ion.warmup.trigger", 90);
> > +var max = 100; // Make have the warm-up counter high enough to
> > +               // consider inlining functions.
> > +
> > +var f4 = SIMD.int32x4; // :TODO: Support float32x4 arith.
> 
> f4, int32x4? okay well this is a test case :)

Yes, but as we don't have Bug 1127932 yet, I needed something which is producing MSimdBox & MSimdUnbox instructions.

> @@ +78,5 @@
> > +for (var i = 0; i < max; i++) {
> > +  var res = test(cardinals);
> > +  assertEqX4(c4norm(res), f4.splat(16 * 16));
> > +  assertEqX4(res.re, cardinals16.re, assertEqDbl);
> > +  assertEqX4(res.im, cardinals16.im, assertEqDbl);
> 
> why not using assertEq here?

Because we need a lose equality between 0 and -0, which does not matter for this test case.

> ::: js/src/jit/MIR.cpp
> @@ +943,5 @@
> > +        if (!phi->reserveLength(boxedPhi->numOperands()))
> > +            return nullptr;
> > +
> > +        for (size_t i = 0, e = boxedPhi->numOperands(); i < e; i++) {
> > +            MDefinition *op = boxedPhi->getOperand(i);
> 
> Shouldn't you check a few more things:
> - op->isSimdBox()
> - op->toSimdBox()->input()->type() == type()

No, because the Phi could be added after an OSR block, and we do not want to Box before a tiny loop just because we have an OSR block in the middle of the script.  Thus GVN should do the propagation of the MSimdUnbox for us.

Hum … but I think I forgot to properly handle one case, where the Phi reference it-self (even indirectly), in which case we want to be reusing the folded result before calling foldsTo.  I also think that we want this to be part as GVN.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8566074 [details] [diff] [review]
Optimize MSimdUnbox with GVN.

Review of attachment 8566074 [details] [diff] [review]:
-----------------------------------------------------------------

Cancelling review per your previous comment.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8566074 [details] [diff] [review]
Optimize MSimdUnbox with GVN.

Review of attachment 8566074 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MIR.cpp
@@ +936,5 @@
> +    }
> +
> +    if (in->isPhi()) {
> +        // If the operand is a MPhi, then we copy the MPhi to an unboxed variant
> +        // where all operands of the MPhi are unboxed.

I will remove this part from this patch for the moment, and then I will experiment with different approaches as soon as we have the RSimdBox implemented.

Comment 6

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46d274a6952e

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/46d274a6952e