SIMD: Fold SimdValueX4 with four same non constant operands into SimdSplatX4

RESOLVED FIXED in mozilla35

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

Trunk
mozilla35
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8489370 [details] [diff] [review]
Fold SimdValueX4 with same non constant operands into SimdSplatX4

Pointers comparison can be done because GVN is done in RPO and at the time of
MSimdValueX4::foldTo, inputs have been replaced by the single equivalent
operation. This is more generic than the approach in AsmJSValidate.cpp, which
wasn't able to use splat for the second pair of test cases added in this patch.
Attachment #8489370 - Flags: review?(sunfish)
Comment on attachment 8489370 [details] [diff] [review]
Fold SimdValueX4 with same non constant operands into SimdSplatX4

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

::: js/src/jit/MIR.cpp
@@ +669,5 @@
>  MSimdValueX4::foldsTo(TempAllocator &alloc)
>  {
>      DebugOnly<MIRType> scalarType = SimdTypeToScalarType(type());
> +    bool allConstants = true,
> +         allSame = true;

Style nit: If you're going to use two lines anyway, just use a semicolon and a new declaration instead of a comma.

@@ +686,5 @@
> +
> +    if (allSame)
> +        return MSimdSplatX4::New(alloc, type(), getOperand(0));
> +
> +    MOZ_ASSERT(allConstants);

It would seem better to do the allConstants check before the allSame check, so that we fold an all-constant value directly to a SimdConstant instead of folding it to a SimdSplat which folds to a SimdConstant.
Attachment #8489370 - Flags: review?(sunfish) → review+
(Assignee)

Comment 3

3 years ago
Thanks for the review (especially for the last comment; i actually thought about that when writing the code, then later found it uglier to have the longer case in the if statement and changed it without thinking about consequences in terms of control flow).

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe424bb7192c
https://hg.mozilla.org/mozilla-central/rev/fe424bb7192c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.