SIMD: implement int32x4 notEqual, lessThanOrEqual, and greaterThanOrEqual

RESOLVED FIXED in mozilla36

Status

()

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

People

(Reporter: sunfish, Unassigned)

Tracking

unspecified
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8494580 [details] [diff] [review]
simd-integer-compares.patch

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+
Created attachment 8526761 [details] [diff] [review]
simd-integer-compares.patch

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)
Created attachment 8526762 [details] [diff] [review]
Simplify comparison tests
Attachment #8526762 - Flags: review?(till)
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+
(Reporter)

Comment 5

3 years ago
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.
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/953e14ecbc02
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/66c241782eed
https://hg.mozilla.org/mozilla-central/rev/953e14ecbc02
https://hg.mozilla.org/mozilla-central/rev/66c241782eed
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.