Closed Bug 1072368 Opened 5 years ago Closed 5 years ago

SIMD: implement int32x4 notEqual, lessThanOrEqual, and greaterThanOrEqual

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: sunfish, Unassigned)

Details

Attachments

(2 files, 1 obsolete file)

Attached patch simd-integer-compares.patch (obsolete) — Splinter Review
This patch implements int32x4 notEqual, lessThanOrEqual, and greaterThanOrEqual, which really ought to be in the spec, as reported here:

https://github.com/johnmccutchan/ecmascript_simd/issues/46

It can wait until the spec change is a little more official; I'm just posting this now because I had a need for it.
Comment on attachment 8494580 [details] [diff] [review]
simd-integer-compares.patch

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

Will need rebasing, but looks good!

::: js/src/builtin/SIMD.h
@@ +81,5 @@
>    V(and, (BinaryFunc<Int32x4, And, Int32x4>), 2, 0)                                 \
>    V(equal, (BinaryFunc<Int32x4, Equal, Int32x4>), 2, 0)                             \
> +  V(notEqual, (BinaryFunc<Int32x4, NotEqual, Int32x4>), 2, 0)                       \
> +  V(lessThan, (BinaryFunc<Int32x4, LessThan, Int32x4>), 2, 0)                       \
> +  V(lessThanOrEqual, (BinaryFunc<Int32x4, LessThanOrEqual, Int32x4>), 2, 0)         \

Reftests, please? /js/src/tests/ecma_6/TypedObjects/simd

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +2392,2 @@
>        case MSimdBinaryComp::greaterThanOrEqual:
> +        // scr := rhs

nit: src (just saw it's pre-existing above, feel free to correct it as well)

@@ +2396,5 @@
> +        else
> +            masm.loadAlignedInt32x4(rhs, ScratchSimdReg);
> +        masm.packedGreaterThanInt32x4(ToOperand(ins->lhs()), ScratchSimdReg);
> +        masm.loadConstantInt32x4(allOnes, lhs);
> +        masm.bitwiseXorX4(Operand(ScratchSimdReg), lhs);

Could you please add a comment that we're computing !(rhs > lhs) here?
Attachment #8494580 - Flags: feedback+
I'd like to have this moving forward, as this has been accepted in the spec, or at least there's general consensus in discussion. Rebased. Tests for the interpreter coming in the next patch.
Attachment #8494580 - Attachment is obsolete: true
Attachment #8526761 - Flags: review?(sunfish)
Comment on attachment 8526762 [details] [diff] [review]
Simplify comparison tests

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

Looks great, I like how many LOCs you were able to get rid of :)

::: js/src/tests/ecma_6/TypedObject/simd/comparisons.js
@@ +1,5 @@
> +// |reftest| skip-if(!this.hasOwnProperty("SIMD"))
> +
> +/*
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/licenses/publicdomain/

Please change this to https://creativecommons.org/publicdomain/zero/1.0/

@@ +63,5 @@
> +      float32x4(13.37, 42.42, NaN, 0)
> +  ];
> +
> +  for (v of float32x4val) {
> +      for (w of float32x4val) {

Technically v and w should be declared somewhere. Not that it matters all that much in a test.

::: js/src/tests/ecma_6/TypedObject/simd/float32x4equal.js
@@ -19,5 @@
> -  assertEq(c.w, 0);
> -
> -  var d = float32x4(1.89, 20.51, 30.46, 40.12);
> -  var e = float32x4(10.89, 20.51, Math.fround(30.46), 4.12);
> -  var f = SIMD.float32x4.equal(d, e);

Huh, so this actually didn't test anything.
Attachment #8526762 - Flags: review?(till) → review+
Comment on attachment 8526761 [details] [diff] [review]
simd-integer-compares.patch

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

::: js/src/builtin/SIMD.h
@@ +181,5 @@
>      _(load)                          \
> +    _(store)                         \
> +    _(lessThanOrEqual)               \
> +    _(notEqual)                      \
> +    _(greaterThanOrEqual)

It'd be tidy to put these next to the other comparisons.

::: js/src/jit-test/tests/asm.js/testSIMD.js
@@ +600,4 @@
>  
> +CheckI4(EQI32, 'var x=i4(1,2,3,4); var y=i4(-1,1,0,2); x=eq(x,y)', [F, F, F, F]);
> +CheckI4(EQI32, 'var x=i4(-1,1,0,2); var y=i4(1,2,3,4); x=eq(x,y)', [F, F, F, F]);
> +CheckI4(EQI32, 'var x=i4(1,0,3,4); var y=i4(1,1,7,0); x=eq(x,y)', [T, F, F, F]);

Were these meant to be lt tests? There are eq tests below, and no lt tests.

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +2676,5 @@
>  
>  bool
>  CodeGeneratorX86Shared::visitSimdBinaryCompIx4(LSimdBinaryCompIx4 *ins)
>  {
> +    static const SimdConstant allOnes = SimdConstant::CreateX4(-1, -1, -1, -1);

This can be SimdConstant::SplatX4(-1);

@@ +2714,4 @@
>        case MSimdBinaryComp::greaterThanOrEqual:
> +        // src := rhs
> +        if (rhs.kind() == Operand::FPREG)
> +            masm.moveAlignedInt32x4(ToFloatRegister(ins->rhs()), ScratchSimdReg);

This predates this patch, but do you know why this function is called "aligned" if it's just doing a register-register copy?
Attachment #8526761 - Flags: review?(sunfish) → review+
(In reply to Dan Gohman [:sunfish] from comment #5)
> @@ +2714,4 @@
> >        case MSimdBinaryComp::greaterThanOrEqual:
> > +        // src := rhs
> > +        if (rhs.kind() == Operand::FPREG)
> > +            masm.moveAlignedInt32x4(ToFloatRegister(ins->rhs()), ScratchSimdReg);
> 
> This predates this patch, but do you know why this function is called
> "aligned" if it's just doing a register-register copy?

I guess I've done that for symmetry with store/load methods, but this could be removed for simplicity.
https://hg.mozilla.org/mozilla-central/rev/953e14ecbc02
https://hg.mozilla.org/mozilla-central/rev/66c241782eed
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.