Closed Bug 1112158 Opened 5 years ago Closed 5 years ago

GVN: Folds MSimdUnbox to MSimdBox operands.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file)

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.
Depends on: 1112159
Depends on: 1112160
Depends on: 1112161
Depends on: 1112162
Depends on: 1112163
Depends on: 1112164
Blocks: 1112165
Depends on: 1130039
Depends on: 1130089
Depends on: 1125561
Depends on: 1130481
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Summary: GVN: Folds useless unbox when SIMD values to their unboxed variant is known. → GVN: Folds MSimdUnbox to MSimdBox operands.
Attachment #8566074 - Flags: review?(benj)
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()
(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 on attachment 8566074 [details] [diff] [review]
Optimize MSimdUnbox with GVN.

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

Cancelling review per your previous comment.
Attachment #8566074 - Flags: review?(benj)
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.
Attachment #8566074 - Flags: review?(benj)
Attachment #8566074 - Flags: review?(benj) → review+
https://hg.mozilla.org/mozilla-central/rev/46d274a6952e
Status: ASSIGNED → 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.