Closed Bug 948379 Opened 11 years ago Closed 10 years ago

Write SIMD tests for border cases of float32 and int32

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36

People

(Reporter: ivan, Assigned: ProgramFOX, Mentored)

References

Details

Attachments

(2 files, 11 obsolete files)

Implemented int32x4 border cases
21.81 KB, patch
ProgramFOX
: review+
Details | Diff | Splinter Review
Added float32x4 border cases
36.33 KB, patch
ProgramFOX
: review+
Details | Diff | Splinter Review
We need to ensure that the SIMD operations have float32 semantics. Tests in js/src/tests/ecma_6/TypedObject/SIMD should be modified to check the correctness of border cases.
Depends on: 946042
In this patch, I implemented the float32x4 border cases in the way bbouvier suggested in Bug 1041648 Comment 3. I did not implement the int32x4 border cases, as I wasn't sure what counts as "border case" there.
Attachment #8462968 - Flags: review?(benj)
Comment on attachment 8462968 [details] [diff] [review] Implemented float32x4 border cases Review of attachment 8462968 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing this! The patch needs rebasing over bug 1042244's changes, sorry about that. Good start! Rather than deleting the currently existing tests, could you add them, in the same files? If you feel like doing so, could you also add tests with vectors that contain NaN, -0, Infinity and -Infinity? Otherwise I guess we'll eventually do this in another bug, when the SIMD spec will be more stable. The interesting border cases for int32x4 are the following: - let INT32_MAX := Math.pow(2, 31) - 1; INT32_MIN := -Math.pow(2, 31); - check that (INT32_MAX + 1) == INT32_MIN, in int32 arithmetic (can be done in Add by adding one, in Sub by subtracting -1, etc.) - check that (INT32_MIN - 1) == INT32_MAX, in int32 arithmetic. It's fine if you don't want to do it: it's easy work but can seem boring at some point. However, if you plan to do it, feel free to make a different patch :) ::: js/src/tests/ecma_6/TypedObject/simd/float32x4add.js @@ +13,2 @@ > var c = SIMD.float32x4.add(a, b); > + assertEq(c.x, Math.fround(11.88)); See comment in float32x4div.js ::: js/src/tests/ecma_6/TypedObject/simd/float32x4div.js @@ +13,2 @@ > var c = SIMD.float32x4.div(b,a); > + assertEq(c.x, Math.fround(8.75)); Does this test pass? For add it's fine to have constant values for the results, but it makes me uncomfortable to also have constants for div. How about having function fdiv(a, b) { return Math.fround(Math.fround(a) / Math.fround(b)); } and use it in the assertEq? (feel free to do that for add and sub as well; please do it for mul) ::: js/src/tests/ecma_6/TypedObject/simd/float32x4equal.js @@ +8,5 @@ > function test() { > print(BUGNUMBER + ": " + summary); > > + var a = float32x4(1.89, 20.51, 30.46, 40.12); > + var b = float32x4(10.89, 20.51, 30.46, 4.12); Could you change the third component of b (z), such that its float representation is the same as 30.46, but it isn't 30.46? (no need to change the assertEq afterwards) ::: js/src/tests/ecma_6/TypedObject/simd/float32x4mul.js @@ +13,2 @@ > var c = SIMD.float32x4.mul(a, b); > + assertEq(c.x, Math.fround(17.7122)); See comment in float32x4div.js ::: js/src/tests/ecma_6/TypedObject/simd/float32x4neg.js @@ +7,5 @@ > > function test() { > print(BUGNUMBER + ": " + summary); > > + var a = float32x4(0.999, -0.001, 3.78, 4.05); It'd be nice to have one of the components to be -0, to check that -(-0) is +0. ::: js/src/tests/ecma_6/TypedObject/simd/float32x4notequal.js @@ +10,5 @@ > > // FIXME -- Bug 948379: Amend to check for correctness of border cases. > > + var a = float32x4(9.98, 20.65, 30.14, 4.235); > + var b = float32x4(9.99, 20.65, 30.14, 4.23); See comment in float32x4equal.js and apply it here as well please. ::: js/src/tests/ecma_6/TypedObject/simd/float32x4reciprocal.js @@ +12,2 @@ > var c = SIMD.float32x4.reciprocal(a); > + assertEq(c.x, Math.fround(0.625)); Could you also use an helper function here? (like for mul/div) ::: js/src/tests/ecma_6/TypedObject/simd/float32x4reciprocalsqrt.js @@ +12,2 @@ > var c = SIMD.float32x4.reciprocalSqrt(a); > + assertEq(c.x, Math.fround(0.2)); Same comment as float32x4reciprocal.js ::: js/src/tests/ecma_6/TypedObject/simd/float32x4scale.js @@ +9,5 @@ > print(BUGNUMBER + ": " + summary); > > + var a = float32x4(1.34, 2.76, 3.21, 4.09); > + var c = SIMD.float32x4.scale(a, 2.54); > + assertEq(c.x, Math.fround(3.4036)); Same comment as mul/div. ::: js/src/tests/ecma_6/TypedObject/simd/float32x4sqrt.js @@ +12,2 @@ > var c = SIMD.float32x4.sqrt(a); > + assertEq(c.x, Math.fround(1.65)); Same comment as mul/div. ::: js/src/tests/ecma_6/TypedObject/simd/float32x4with.js @@ +12,5 @@ > + var x = SIMD.float32x4.withX(a, 5.38); > + var y = SIMD.float32x4.withY(a, 5.19); > + var z = SIMD.float32x4.withZ(a, 5.11); > + var w = SIMD.float32x4.withW(a, 5.07); > + assertEq(x.x, Math.fround(5.38)); While you're here: it'd be nice to test that other values haven't changed, e.g. for x: assertEq(x.y, Math.fround(2.08)); assertEq(x.z, Math.fround(3.84)); assertEq(x.w, Math.fround(4.17));
Attachment #8462968 - Flags: review?(benj) → feedback+
This is the patch containing the border cases for the float32x4 methods (in the float32x4*.js) files. Many of the NaN/-0/Infinity/-Infinity tests fail. In the following files, I have verified with the JS shell, and my data in assertEq appears to be correct: float32x4abs.js float32x4and.js float32x4max.js float32x4min.js float32x4not.js float32x4or.js float32x4reciprocalsqrt.js float32x4xor.js For float32x4fromint32x4bits.js, there is a different explanation. This one fails at: >assertEq(f.z, 0); The int32x4 value was initially set to -0, but this gets changed into 0 when creating it (verified that using shell), and for some reason it returns -0 in the resulting float32x4. I'm not sure about: float32x4equal.js (fails at NaN, the shell gives false for NaN === NaN, but that's the correct result, so I'm not sure about the test case here) float32x4notequal.js (same as equal) float32x4lessthanorequal.js (same as equal) float32x4greaterthanorequal.js (same as equal) float32x4fromint32x4.js (I used INT32_MAX (Math.pow(2, 31) - 1) in the int32x4, but got Math.pow(2, 31) in the resulting float32x4)
Attachment #8462968 - Attachment is obsolete: true
Attachment #8468554 - Flags: review?(benj)
Attached patch Implemented int32x4 border cases (obsolete) — — Splinter Review
This patch contains the border cases for int32x4. Because the border cases were about adding/substracting by 1 and converting to int32x4, I assumed that I didn't have to change anything about the other test cases, and I just removed the FIXME comment there. Let me know if I was wrong here. The int32x4fromfloat32x4.js test case fails now, but I checked it using the straw man proposal, and my assertEq data appears to be correct.
Attachment #8469165 - Flags: review?(benj)
Comment on attachment 8468554 [details] [diff] [review] Implemented float32x4 border cases Review of attachment 8468554 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for writing these! I've looked at some of the failing test cases, and you might have found some issues in the implementation. It's probably due to the same design issue that arose in the float64x2 implementation, which is that the CoerceFunc isn't always applied on the correct type. I'll try this with my local patch queue which should fix that and will let you know. ::: js/src/tests/ecma_6/TypedObject/simd/float32x4and.js @@ +37,5 @@ > + > + var g = float32x4(NaN, -0, Infinity, -Infinity); > + var h = float32x4(NaN, -0, -Infinity, Infinity); > + var i = SIMD.float32x4.and(g, h); > + assertEq(i.x, 0); Are you sure about these? ::: js/src/tests/ecma_6/TypedObject/simd/float32x4equal.js @@ +26,5 @@ > + > + var g = float32x4(NaN, -0, Infinity, -Infinity); > + var h = float32x4(NaN, 0, Infinity, Infinity); > + var i = SIMD.float32x4.equal(g, h); > + assertEq(i.x, -1); The x86 instruction for float32x4.equal (CMPPS) returns false for NaN == NaN, which is the behavior specified in IEEE754, so that's expected that it should be 0. Same for other comparisons involving "or equal". ::: js/src/tests/ecma_6/TypedObject/simd/float32x4fromint32x4.js @@ +19,5 @@ > assertEq(c.w, 4); > > + var d = int32x4(INT32_MIN, INT32_MAX, -0, NaN); > + var f = SIMD.float32x4.fromInt32x4(d); > + assertEq(f.x, INT32_MIN); Some Int32 can't be represented precisely as Float32s; this is the case for INT32_MAX, which is represented as 2**31, so this assert will fail. How about writing these tests like this: assertEq(f.x, Math.fround(INT32_MIN)); // and so on so that you don't have to worry about these representation details? ::: js/src/tests/ecma_6/TypedObject/simd/float32x4fromint32x4bits.js @@ +17,5 @@ > assertEq(c.y, 2.802596928649634e-43); > assertEq(c.z, 4.203895392974451e-43); > assertEq(c.w, 5.605193857299268e-43); > > + var d = int32x4(INT32_MIN, INT32_MAX, -0, NaN); -0 and NaN as int32 are coerced into 0, so that's not worth using them here. @@ +19,5 @@ > assertEq(c.w, 5.605193857299268e-43); > > + var d = int32x4(INT32_MIN, INT32_MAX, -0, NaN); > + var f = SIMD.float32x4.fromInt32x4Bits(d); > + assertEq(f.x, 0); This one should be -0, as INT32_MIN is negative, its sign bit is set, and the sign bit is the same for int32s and float32s.
Attachment #8468554 - Flags: review?(benj) → feedback+
Comment on attachment 8469165 [details] [diff] [review] Implemented int32x4 border cases Review of attachment 8469165 [details] [diff] [review]: ----------------------------------------------------------------- Nice tests! There are still other operations craving for more tests :) ::: js/src/tests/ecma_6/TypedObject/simd/int32x4add.js @@ +16,5 @@ > assertEq(c.z, 33); > assertEq(c.w, 44); > > + var INT32_MAX = Math.pow(2, 31) - 1; > + var INT32_MIN = -Math.pow(2, 31); Can you add a blank line here please? @@ +19,5 @@ > + var INT32_MAX = Math.pow(2, 31) - 1; > + var INT32_MIN = -Math.pow(2, 31); > + var d = int32x4(INT32_MAX, INT32_MIN, INT32_MAX, INT32_MIN); > + var e = int32x4(1, -1, 0, 0); > + var f = SIMD.int32x4.add(d, e); And here too? ::: js/src/tests/ecma_6/TypedObject/simd/int32x4fromfloat32x4.js @@ +16,5 @@ > assertEq(c.w, 4); > > + var d = float32x4(NaN, -0, Infinity, -Infinity); > + var f = SIMD.int32x4.fromFloat32x4(d); > + assertEq(f.x, 0); Might be an issue in the interpreter, if it's not behaving correctly. I'll look into that. ::: js/src/tests/ecma_6/TypedObject/simd/int32x4fromfloat32x4bits.js @@ +19,5 @@ > + var f = SIMD.int32x4.fromFloat32x4Bits(d); > + assertEq(f.x, 2143289344); > + assertEq(f.y, -2147483648); > + assertEq(f.z, 2139095040); > + assertEq(f.w, -8388608); Nice test! ::: js/src/tests/ecma_6/TypedObject/simd/int32x4lsh.js @@ -7,5 @@ > > function test() { > print(BUGNUMBER + ": " + summary); > > - // FIXME -- Bug 948379: Amend to check for correctness of border cases. Could you add a test x4 that checks: - INT32_MIN << 1 === 0 - INT32_MAX << 1 === -2 ::: js/src/tests/ecma_6/TypedObject/simd/int32x4mul.js @@ -7,5 @@ > > function test() { > print(BUGNUMBER + ": " + summary); > > - // FIXME -- Bug 948379: Amend to check for correctness of border cases. Could you add a test that involves -1 and INT32_MIN, and -1 and INT32_MAX? ::: js/src/tests/ecma_6/TypedObject/simd/int32x4neg.js @@ -7,5 @@ > > function test() { > print(BUGNUMBER + ": " + summary); > > - // FIXME -- Bug 948379: Amend to check for correctness of border cases. Same as for int32x4mul, as neg(x) === x * (-1) ::: js/src/tests/ecma_6/TypedObject/simd/int32x4or.js @@ -7,5 @@ > > function test() { > print(BUGNUMBER + ": " + summary); > > - // FIXME -- Bug 948379: Amend to check for correctness of border cases. Could you add a test involving INT32_MAX | INT32_MIN? ::: js/src/tests/ecma_6/TypedObject/simd/int32x4rsh.js @@ -7,5 @@ > > function test() { > print(BUGNUMBER + ": " + summary); > > - // FIXME -- Bug 948379: Amend to check for correctness of border cases. Could you add a test involving INT32_MIN >> 1, and INT32_MAX >> 1? ::: js/src/tests/ecma_6/TypedObject/simd/int32x4sub.js @@ +16,5 @@ > assertEq(c.z, 27); > assertEq(c.w, 36); > > + var INT32_MAX = Math.pow(2, 31) - 1; > + var INT32_MIN = -Math.pow(2, 31); nit: can you add blank lines please? ::: js/src/tests/ecma_6/TypedObject/simd/int32x4ursh.js @@ -7,5 @@ > > function test() { > print(BUGNUMBER + ": " + summary); > > - // FIXME -- Bug 948379: Amend to check for correctness of border cases. Could you add a test involving INT32_MIN >>> 0?
Attachment #8469165 - Flags: review?(benj) → feedback+
(In reply to ProgramFOX (on vacation Aug 8 - Aug 16, not checking email) from comment #3) > Created attachment 8468554 [details] [diff] [review] > Implemented float32x4 border cases > > This is the patch containing the border cases for the float32x4 methods (in > the float32x4*.js) files. > > Many of the NaN/-0/Infinity/-Infinity tests fail. In the following files, I > have verified with the JS shell, and my data in assertEq appears to be > correct: > > float32x4abs.js > float32x4and.js > float32x4max.js > float32x4min.js > float32x4not.js > float32x4or.js > float32x4reciprocalsqrt.js > float32x4xor.js I've looked at some of the tests: apparently for min/max, in the ES6 spec [1] there is some rule that if any of the inputs is NaN, then the result should be NaN. This isn't checked in the UnaryFunc functions. I think this is an issue about the specification of SIMD.js, so I've filed an issue on github [2]. [1] http://people.mozilla.org/~jorendorff/es6-draft.html#sec-math.min [2] https://github.com/johnmccutchan/ecmascript_simd/issues/51
Status: UNCONFIRMED → NEW
Ever confirmed: true
I updated the test cases. Yes, I'm sure about the AND operation. When a bitwise operation is performed, the numbers are converted to an Int32 [1], which returns +0 if the input is NaN, +0, −0, +Infinity, or −Infinity [2]. [1]:https://people.mozilla.org/~jorendorff/es6-draft.html#sec-binary-bitwise-operators-runtime-semantics-evaluation [2]:https://people.mozilla.org/~jorendorff/es6-draft.html#sec-toint32
Attachment #8468554 - Attachment is obsolete: true
Attachment #8474486 - Flags: review?(benj)
Attached patch Implemented int32x4 border cases (obsolete) — — Splinter Review
This is a new patch for the int32x4 border cases, with the other border cases added. The test cases fail at int32x4fromfloat32x4, int32x4mul, int32x4neg and int32x4ursh.
Attachment #8469165 - Attachment is obsolete: true
Attachment #8475958 - Flags: review?(benj)
Comment on attachment 8474486 [details] [diff] [review] Implemented float32x4 border cases Review of attachment 8474486 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to ProgramFOX from comment #8) > Created attachment 8474486 [details] [diff] [review] > Implemented float32x4 border cases > > I updated the test cases. > > Yes, I'm sure about the AND operation. When a bitwise operation is > performed, the numbers are converted to an Int32 [1], which returns +0 if > the input is NaN, +0, −0, +Infinity, or −Infinity [2]. > So semantics of this operation are different in the SIMD and the non-SIMD version. The SIMD version is just a bit-and operation, while the non SIMD variant first applies ToInt32 and so on. Can you please update your tests to reflect this? Also, it's been said (can't find the reference anywhere but I am 100% sure) that Float32x4 operators should behave like the Intel implementation, more than like the JS version. In any cases, that is not standardized yet, so I would even be in favor with delaying this patch.
Attachment #8474486 - Flags: review?(benj)
Comment on attachment 8475958 [details] [diff] [review] Implemented int32x4 border cases Review of attachment 8475958 [details] [diff] [review]: ----------------------------------------------------------------- I am sorry but a patch with test cases that fail automatically won't get a positive review. Could you set up a debugger and try to see what's going up? Is is that the functions are wrongly implemented? Can you identify the first place where things reliably turn wrong? Are the tests testing correct things? Don't forget that you're working in the Int32 space, which cannot represent all values without overflowing or underflowing. If you want to test int32 arithmetic in the interpreter, you can use the bitwise |0 operator, that converts its input to an int32. With this operator, you should be able to check what's wrong with the tests. For instance, giving a quick look at int32x4mul.js, you could write in the interpreter assertEq(-1 * INT32_MIN | 0, -INT32_MIN) and see it fail. The reason is -1 * INT32_MIN == -(-Math.pow(2,31)) == Math.pow(2,31) > INT32_MAX == Math.pow(2,31) - 1, so it overflows by one. As a matter of fact, in 32 bits arithmetic, -INT32_MIN == INT32_MIN.
Attachment #8475958 - Flags: review?(benj)
Attached patch Implemented int32x4 border cases (obsolete) — — Splinter Review
I improved the test cases and used |0 to get the correct results and ConvertScalar to successfully convert NaN, Infinity and -Infinity to an int32. For the float32x4 border cases, I'll wait until the behavior for NaN/Infinity/-Infinity/-0 is standardized, as you said in your comments.
Attachment #8475958 - Attachment is obsolete: true
Attachment #8483508 - Flags: review?(benj)
Comment on attachment 8483508 [details] [diff] [review] Implemented int32x4 border cases Review of attachment 8483508 [details] [diff] [review]: ----------------------------------------------------------------- Nice! Sorry I had a few more ideas for binary operators: let M = INT32_MAX, m = INT32_MIN, rather than testing (M,m,M,m) against (m,M,m,M), we can test (M,m,M,m) against (m,M,M,m) and have more coverage. ::: js/src/tests/ecma_6/TypedObject/simd/int32x4and.js @@ +19,5 @@ > + var INT32_MAX = Math.pow(2, 31) - 1; > + var INT32_MIN = -Math.pow(2, 31); > + > + var d = int32x4(INT32_MAX, INT32_MIN, INT32_MAX, INT32_MIN); > + var e = int32x4(INT32_MIN, INT32_MAX, INT32_MIN, INT32_MAX); rather than having the last two lanes being the symmetric of the first two, how about putting INT32_MAX in the 3rd lane for both values, and INT32_MIN in the 4th lane for both values? ::: js/src/tests/ecma_6/TypedObject/simd/int32x4fromfloat32x4.js @@ +7,5 @@ > > function test() { > print(BUGNUMBER + ": " + summary); > > + var INT32_MIN = -Math.pow(2, 31); nit: unused ::: js/src/tests/ecma_6/TypedObject/simd/int32x4fromfloat32x4bits.js @@ +16,5 @@ > assertEq(c.w, 1082130432); > > + var d = float32x4(NaN, -0, Infinity, -Infinity); > + var f = SIMD.int32x4.fromFloat32x4Bits(d); > + assertEq(f.x, 2143289344); pre-existing: I just realized that having the values in hexadecimal form in these assertEq would make it clearer that it's all about bit patterns. Could you please update them to be in hexa form? (i.e. rather than -2147483648, use 0x80000000 | 0) ::: js/src/tests/ecma_6/TypedObject/simd/int32x4lsh.js @@ +18,5 @@ > } > > + var INT32_MAX = Math.pow(2, 31) - 1; > + var INT32_MIN = -Math.pow(2, 31); > + nit: trailing space @@ +21,5 @@ > + var INT32_MIN = -Math.pow(2, 31); > + > + var d = int32x4(INT32_MAX, INT32_MIN, INT32_MAX, INT32_MIN); > + var f = SIMD.int32x4.shiftLeft(d, 1); > + assertEq(f.x, -2); could you put (INT32_MAX << 1)|0 here, instead? and ditto for the following values ::: js/src/tests/ecma_6/TypedObject/simd/int32x4mul.js @@ +19,5 @@ > + var INT32_MAX = Math.pow(2, 31) - 1; > + var INT32_MIN = -Math.pow(2, 31); > + > + var d = int32x4(INT32_MAX, INT32_MIN, INT32_MAX, INT32_MIN); > + var e = int32x4(-1, -1, -1, -1); same here: put INT32_MIN and INT32_MAX in e's two last lanes ::: js/src/tests/ecma_6/TypedObject/simd/int32x4neg.js @@ +17,5 @@ > > + var INT32_MAX = Math.pow(2, 31) - 1; > + var INT32_MIN = -Math.pow(2, 31); > + > + var d = int32x4(INT32_MAX, INT32_MIN, INT32_MAX, INT32_MIN); could you test that -0 === 0 in int32 arithmetic? ::: js/src/tests/ecma_6/TypedObject/simd/int32x4not.js @@ -8,5 @@ > function test() { > print(BUGNUMBER + ": " + summary); > > - // FIXME -- Bug 948379: Amend to check for correctness of border cases. > - can you add a test that uses int32 min / max and 0? ::: js/src/tests/ecma_6/TypedObject/simd/int32x4or.js @@ +17,5 @@ > assertEq(c.w, 44); > > + var INT32_MAX = Math.pow(2, 31) - 1; > + var INT32_MIN = -Math.pow(2, 31); > + nit: trailing space @@ +19,5 @@ > + var INT32_MAX = Math.pow(2, 31) - 1; > + var INT32_MIN = -Math.pow(2, 31); > + > + var d = int32x4(INT32_MAX, INT32_MIN, INT32_MAX, INT32_MIN); > + var e = int32x4(INT32_MIN, INT32_MAX, INT32_MIN, INT32_MAX); same here: can you make e's last 2 components be the same as d? ::: js/src/tests/ecma_6/TypedObject/simd/int32x4sub.js @@ +18,5 @@ > > + var INT32_MAX = Math.pow(2, 31) - 1; > + var INT32_MIN = -Math.pow(2, 31); > + > + var d = int32x4(-1, 1, 0, 0); same here: can you make d's last two components be the same as e's? ::: js/src/tests/ecma_6/TypedObject/simd/int32x4with.js @@ +11,5 @@ > var a = int32x4(1, 2, 3, 4); > var x = SIMD.int32x4.withX(a, 5); > var y = SIMD.int32x4.withY(a, 5); > var z = SIMD.int32x4.withZ(a, 5); > var w = SIMD.int32x4.withW(a, 5); could you please change this line to var w = SIMD.int32x4.withW(a, INT32_MAX + 1); and modify the assertEq below? ::: js/src/tests/ecma_6/TypedObject/simd/int32x4xor.js @@ +19,5 @@ > + var INT32_MAX = Math.pow(2, 31) - 1; > + var INT32_MIN = -Math.pow(2, 31); > + > + var d = int32x4(INT32_MAX, INT32_MIN, INT32_MAX, INT32_MIN); > + var e = int32x4(INT32_MIN, INT32_MAX, INT32_MIN, INT32_MAX); same here: please change e's last two lines to be INT32_MAX and INT32_MIN resp. and modify the corresponding assertEq.
Attachment #8483508 - Flags: review?(benj) → feedback+
Attached patch Implemented int32x4 border cases (obsolete) — — Splinter Review
Attachment #8483508 - Attachment is obsolete: true
Attachment #8483605 - Flags: review?(benj)
Comment on attachment 8483605 [details] [diff] [review] Implemented int32x4 border cases Review of attachment 8483605 [details] [diff] [review]: ----------------------------------------------------------------- Last round of comments, but I think we're good to go! Thanks! Please put an updated patch and push it to try, and I'll land it for you. ::: js/src/tests/ecma_6/TypedObject/simd/int32x4mul.js @@ +19,5 @@ > + var INT32_MAX = Math.pow(2, 31) - 1; > + var INT32_MIN = -Math.pow(2, 31); > + > + var d = int32x4(INT32_MAX, INT32_MIN, INT32_MAX, INT32_MIN); > + var e = int32x4(-1, -1, INT32_MIN, INT32_MAX); nit: the 2 last components test the same thing. Can you have INT32_MIN multiply itself on the last component? (i.e. replace e's last lane by INT32_MIN) ::: js/src/tests/ecma_6/TypedObject/simd/int32x4sub.js @@ +23,5 @@ > + var e = int32x4(INT32_MAX, INT32_MIN, INT32_MAX, INT32_MIN); > + var f = SIMD.int32x4.sub(e, d); > + assertEq(f.x, (INT32_MAX - -1) | 0); > + assertEq(f.y, (INT32_MIN - 1) | 0); > + assertEq(f.z, (INT32_MAX - INT32_MAX) | 0); in this case, for the last two lanes you can just put 0 :) ::: js/src/tests/ecma_6/TypedObject/simd/int32x4ursh.js @@ +21,5 @@ > + var INT32_MIN = -Math.pow(2, 31); > + > + var d = int32x4(INT32_MAX, INT32_MIN, INT32_MAX, INT32_MIN); > + var f = SIMD.int32x4.shiftRightLogical(d, 0); > + assertEq(f.x, INT32_MAX); please make it (INT32_MAX >>> 0) | 0, and same for the next lines
Attachment #8483605 - Flags: review?(benj) → review+
Fixed the last remarks, and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3cbecb184afc
Attachment #8483605 - Attachment is obsolete: true
Attachment #8485375 - Flags: review+
The try push succeeded for almost all platforms, except for Windows 2012 x64, because the Mozconfig check failed. What can I do to solve this?
Flags: needinfo?(benj)
Win64 is broken and unsupported, hence why it isn't run by default :-)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #18) > Win64 is broken and unsupported, hence why it isn't run by default :-) Good to know, thanks!
Flags: needinfo?(benj)
Blocks: 1063946
No longer blocks: 1063946
About the float test cases file, I just realized that all test which don't involve comparisons and conversions from float to int32 *should* pass. If that's the case, you could split the patch into two parts: 1) the tests that pass because semantics are clear, 2) tests that don't pass because the semantics aren't well specified yet (for these test files, you can change the TODO bug number to refer to bug 1068028). How does that sound?
Here is the updated patch. For now, I removed the NaN/-0/+Infinity/-Infinity border cases from the bitwise operations test cases, because I wasn't entirely sure how to make them work. For example, in float32x4xor.js, I got `-5.877471754111438e-39` for `-Infinity ^ NaN`, which looks incorrect to me.
Attachment #8474486 - Attachment is obsolete: true
Attachment #8496590 - Flags: feedback?(benj)
Comment on attachment 8496590 [details] [diff] [review] Implemented float32x4 border cases (work in progress) Review of attachment 8496590 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to ProgramFOX from comment #23) > Created attachment 8496590 [details] [diff] [review] > Implemented float32x4 border cases (work in progress) . For example, in float32x4xor.js, I got > `-5.877471754111438e-39` for `-Infinity ^ NaN`, which looks incorrect to me. This is actually correct. We are operating on the bits here, so the resulting values shall not make a lot of sense. So you can add all tests to the bitwise operators. Moreover, only NaN is an issue in the spec; you can amend all tests to use +/-Infinity, even the ones involving comparisons. ::: js/src/builtin/SIMD.cpp @@ +737,5 @@ > namespace js { > // Unary SIMD operators > template<typename T> > struct Abs { > + static inline T apply(T x) { return IsNegativeZero(x) ? 0 : (x < 0 ? -1 * x : x); } Can you make this function actually just return Abs(x), where Abs is mozilla::Abs? ::: js/src/tests/ecma_6/TypedObject/simd/float32x4abs.js @@ +15,5 @@ > assertEq(c.z, 3); > assertEq(c.w, 4); > > + var d = float32x4(-1.63, 2.46, -3.17, 4.94); > + var f = SIMD.float32x4.abs(d); uber-nit: While you're here, can you use the float32x4 renaming here? i.e. write var f = float32x4.abs(d); Here, and for every other test in this file, and every other test file please? ::: js/src/tests/ecma_6/TypedObject/simd/float32x4and.js @@ +33,5 @@ > + assertEq(f.x, andf(1.51, 10.29)); > + assertEq(f.y, andf(2.98, 20.12)); > + assertEq(f.z, andf(3.65, 30.79)); > + assertEq(f.w, andf(4.34, 40.41)); > + Please add test cases for edge values (NaN, Infinity...) ::: js/src/tests/ecma_6/TypedObject/simd/float32x4clamp.js @@ +26,5 @@ > + assertEq(f.z, Math.fround(50.13)); > + assertEq(f.w, Math.fround(0.54)); > + > + var g = float32x4(NaN, 0, 10, -10); > + var lower2 = float32x4(NaN, -0, Infinity, -Infinity); clamp involves comparisons, so this test using NaN should be written later. Please skip the NaN values and just use +/- Infinity instead and don't test with upper == lower. ::: js/src/tests/ecma_6/TypedObject/simd/float32x4div.js @@ +28,5 @@ > + assertEq(f.z, divf(29.1957, 3.17)); > + assertEq(f.w, divf(46.4049, 4.59)); > + > + var g = float32x4(NaN, -0, Infinity, -Infinity); > + var h = float32x4(NaN, -0, -Infinity, Infinity); Could you check that 1 / -0 === -Infinity, and 1 / 0 === Infinity, instead of NaN / NaN and Infinity / -Infinity? ::: js/src/tests/ecma_6/TypedObject/simd/float32x4equal.js @@ +23,5 @@ > + var f = SIMD.float32x4.equal(d, e); > + assertEq(c.x, 0); > + assertEq(c.y, -1); > + assertEq(c.z, -1); > + assertEq(c.w, 0); NaN != NaN, so you can remove the FIXME comment here and just implement all missing tests. ::: js/src/tests/ecma_6/TypedObject/simd/float32x4fromint32x4.js @@ +17,5 @@ > assertEq(c.y, 2); > assertEq(c.z, 3); > assertEq(c.w, 4); > > + var d = int32x4(INT32_MIN, INT32_MAX, -0, NaN); The two last values will be converted into an int32 when constructing d, and in this particular case, converted to 0. This doesn't effectively test float32x4fromint32x4, so please just change these two values by two other values :) What would be great is to have one of these two values be another int32 that can't be represented as a float32 exactly, for instance Math.pow(2, 30) - 1. ::: js/src/tests/ecma_6/TypedObject/simd/float32x4fromint32x4bits.js @@ +24,5 @@ > + assertEq(f.y, NaN); > + assertEq(f.z, 0); > + assertEq(f.w, 0); > + > + nit: 2 blank lines, please remove one here. ::: js/src/tests/ecma_6/TypedObject/simd/float32x4notequal.js @@ +23,5 @@ > + var f = SIMD.float32x4.notEqual(d, e); > + assertEq(f.x, -1); > + assertEq(f.y, 0); > + assertEq(f.z, 0); > + assertEq(f.w, -1); NaN != NaN, so you should be able to implement all cases here as well. ::: js/src/tests/ecma_6/TypedObject/simd/float32x4scale.js @@ +26,5 @@ > + assertEq(f.z, mulf(3.21, 2.54)); > + assertEq(f.w, mulf(4.09, 2.54)); > + > + var g = float32x4(NaN, -0, Infinity, -Infinity); > + var i = SIMD.float32x4.scale(g, Infinity); Can you scale by a fixed number, say 13.37 instead? ::: js/src/tests/ecma_6/TypedObject/simd/float32x4sqrt.js @@ +18,5 @@ > assertEq(c.y, 2); > assertEq(c.z, 3); > assertEq(c.w, 4); > > + var d = float32x4(2.7225, 7.3441, 9.4249, 19.8916); Could you have one of the numbers be a negative number, and check that its sqrt is NaN? @@ +31,5 @@ > + assertEq(i.x, NaN); > + assertEq(i.y, -0); > + assertEq(i.z, Infinity); > + assertEq(i.w, NaN); > + nit: trailing whitespace
Attachment #8496590 - Flags: feedback?(benj)
Here is an updated patch. Currently, only the comparison test cases fail. I tried to solve this by adding extra instructions in SIMD.cpp (I had to use different structs for this, otherwise I got build errors), but it seems like these instructions just get ignored (even if I updated SIMD.h). I use IsNegativeZero/IsNaN to check wheter a value is -0 or NaN before executing code in an if statement, but even if it is -0 or NaN, it does not execute the code in that if statement.
Attachment #8496590 - Attachment is obsolete: true
Attachment #8498305 - Flags: feedback?(benj)
Comment on attachment 8498305 [details] [diff] [review] Added float32x4 border cases (Work In Progress) Review of attachment 8498305 [details] [diff] [review]: ----------------------------------------------------------------- In jsmath.cpp, there are min_double and max_double, you can also make them public and use these in Minimum and such, for the float type. However, as suggested in previous comments, I am fine if you just keep for later tests that involve comparisons (lessThan, lessThanOrEqual, greaterThan, greaterThanOrEqual, equal, notEqual, min, max) and implement tests for the other operations first. Let's start by doing that, and worry about comparisons in either another patch or another bug, ok? I'd like to help landing tests for the other operations, at least.
Attachment #8498305 - Flags: feedback?(benj)
See Also: → 1081697
Attached patch Added float32x4 border cases (obsolete) — — Splinter Review
Here are the updated float32x4 border cases, without the comparison-related NaN/-0/Infinity/-Infinity border cases.
Attachment #8498305 - Attachment is obsolete: true
Attachment #8504903 - Flags: review?(benj)
Comment on attachment 8504903 [details] [diff] [review] Added float32x4 border cases Review of attachment 8504903 [details] [diff] [review]: ----------------------------------------------------------------- Good, thanks! I'd like to move on on this one, so r+ing it with nits (which means you should address them before updating your patch). You may need to rebase your patch, as shuffle and shuffleMix don't exist anymore, so you can just delete your changes in the corresponding test files. Once that's done, post an updated version with r=bbouvier in the commit message, carry forward r+ and post a try link please (jsreftests on all platforms). Thanks! ::: js/src/tests/ecma_6/TypedObject/simd/float32x4not.js @@ +33,5 @@ > + assertEq(f.w, notf(5.28)); > + > + var g = float32x4(NaN, -0, Infinity, -Infinity); > + var i = SIMD.float32x4.not(g); > + assertEq(i.x, -5.877470352812973e-39); assertEq(i.x, notf(NaN)); // etc below ::: js/src/tests/ecma_6/TypedObject/simd/float32x4or.js @@ +37,5 @@ > + > + var g = float32x4(NaN, -0, Infinity, -Infinity); > + var h = float32x4(5, 5, -Infinity, Infinity); > + var i = SIMD.float32x4.or(g, h); > + assertEq(i.x, NaN); assertEq(i.x, orf(NaN, 5)); // etc below ::: js/src/tests/ecma_6/TypedObject/simd/float32x4xor.js @@ +37,5 @@ > + > + var g = float32x4(NaN, -0, Infinity, -Infinity); > + var h = float32x4(-0, Infinity, -Infinity, NaN); > + var i = SIMD.float32x4.xor(g, h); > + assertEq(i.x, NaN); assertEq(i.x, xorf(NaN, -0)); // etc below
Attachment #8504903 - Flags: review?(benj) → review+
Assignee: nobody → programfox
Blocks: 1081697
Mentor: benj
Status: NEW → ASSIGNED
Keywords: leave-open
See Also: 1081697 →
Attached patch Added float32x4 border cases (obsolete) — — Splinter Review
Updated patch with the remarks fixed. Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=932488c4e672
Attachment #8504903 - Attachment is obsolete: true
Attachment #8509471 - Flags: review+
Comment on attachment 8509471 [details] [diff] [review] Added float32x4 border cases Review of attachment 8509471 [details] [diff] [review]: ----------------------------------------------------------------- Nope, you didn't address the nits. I assume you only uploaded the wrong patch and put the wrong patch to try, so i've canceled the try build on your behalf.
Attachment #8509471 - Flags: review+
Some jobs were not yet finished after two days and they are now cancelled, but RyanVM said on #developers that the push looks fine as it is now, so I added checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8f5051232a06
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: