Closed
Bug 1072368
Opened 10 years ago
Closed 9 years ago
SIMD: implement int32x4 notEqual, lessThanOrEqual, and greaterThanOrEqual
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: sunfish, Unassigned)
Details
Attachments
(2 files, 1 obsolete file)
10.20 KB,
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
13.13 KB,
patch
|
till
:
review+
|
Details | Diff | 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 1•10 years ago
|
||
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+
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
Attachment #8526762 -
Flags: review?(till)
Comment 4•10 years ago
|
||
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•10 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+
Comment 6•9 years ago
|
||
(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.
Comment 7•9 years ago
|
||
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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•