Closed
Bug 1123777
Opened 9 years ago
Closed 9 years ago
SIMD splat and with* functions should coerce their arguments
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: sunfish, Assigned: bbouvier)
Details
Attachments
(2 files, 1 obsolete file)
14.30 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
4.00 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
The SIMD polyfill for the splat and with* functions coerces the scalar arguments to the SIMD element type, so that e.g. SIMD.float32x4.splat(undefined) returns a vector with all NaNs. However, the SpiderMonkey implementations throw a TypeError if the scalar arguments are not numbers: js> SIMD.int32x4.splat(undefined) typein:14:0 TypeError: invalid arguments js> SIMD.float32x4.splat(undefined) typein:15:0 TypeError: invalid arguments js> SIMD.int32x4.withY(SIMD.int32x4(2,2,2,2), undefined) typein:1:0 TypeError: invalid arguments js> SIMD.float32x4.withY(SIMD.float32x4(2,2,2,2), undefined) typein:1:0 TypeError: invalid arguments This is also inconsistent with the 4-argument constructor functions, which currently do coerce their scalar arguments. Coercing the arguments seems to me to be more idiomatic than throwing a TypeError, for these functions, however I'm open to other opinions.
Assignee | ||
Comment 1•9 years ago
|
||
Looks reasonable to me. I need to check the behavior in Odin as well.
Assignee: nobody → benj
Status: NEW → ASSIGNED
Flags: needinfo?(benj)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8558670 -
Flags: review?(luke)
Assignee | ||
Comment 3•9 years ago
|
||
Also factors out the with tests. And adds tests for splat, which were not existing (!).
Attachment #8558671 -
Flags: review?(till)
Assignee | ||
Updated•9 years ago
|
Attachment #8558670 -
Attachment is obsolete: true
Attachment #8558670 -
Flags: review?(luke)
Assignee | ||
Comment 4•9 years ago
|
||
Seems that bzexport only works with mq, not named branches.
Flags: needinfo?(benj)
Attachment #8558672 -
Flags: review?(luke)
Comment 5•9 years ago
|
||
Comment on attachment 8558672 [details] [diff] [review] asmjs-with.patch Review of attachment 8558672 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/asmjs/AsmJSValidate.cpp @@ +5419,5 @@ > } > > // Second argument is the scalar > + CheckSimdScalarArgs scalarChecker(formalSimdType_); > + return scalarChecker(f, arg, argIndex, actualType, def); How about: return CheckSimdScalarArgs(formalSimdType_)(f, arg, argIndex, actualType, def); ?
Attachment #8558672 -
Flags: review?(luke) → review+
Comment 6•9 years ago
|
||
Sorry for the late response here. It'll still be a few days until I get to this, so if waiting for that would be annoying, you should shift the review to someone else. (I'm happy to do it then, though.)
Comment 7•9 years ago
|
||
Comment on attachment 8558671 [details] [diff] [review] Have SIMD shifts + splat + with functions apply ToInt32 to their scalar arguments Review of attachment 8558671 [details] [diff] [review]: ----------------------------------------------------------------- That was a small change indeed. Sorry again for the delay. ::: js/src/builtin/SIMD.h @@ +257,5 @@ > return a; > } > static bool toType(JSContext *cx, JS::HandleValue v, Elem *out) { > + double d; > + bool res = ToNumber(cx, v, &d); I'd just do `if (!ToNumber(cx, v, &d)) return false`, but uber-nit, so up to you. ::: js/src/tests/ecma_7/SIMD/shifts.js @@ +20,5 @@ > function test() { > + function TestError() {}; > + > + var good = {valueOf: () => 21}; > + var bad = {valueOf: () => {throw new TestError;}}; Not that I care much, but I think SpiderMonkey-style is to always use braces for ctors. @@ +32,5 @@ > testBinaryScalarFunc(v, bits, int32x4.shiftLeftByScalar, lsh); > testBinaryScalarFunc(v, bits, int32x4.shiftRightArithmeticByScalar, rsh); > testBinaryScalarFunc(v, bits, int32x4.shiftRightLogicalByScalar, ursh); > } > + // Test that the shift count is coerced to an int32 Uber-nit: dot missing at the end. ::: js/src/tests/ecma_7/SIMD/splat.js @@ +19,5 @@ > +function test() { > + function TestError(){}; > + > + var good = {valueOf: () => 19.89}; > + var bad = {valueOf: () => { throw new TestError }}; Same nit as in shifts.js. ::: js/src/tests/ecma_7/SIMD/with.js @@ +47,5 @@ > + } > + > + function TestError(){}; > + var good = {valueOf: () => 42}; > + var bad = {valueOf: () => {throw new TestError}}; Same here.
Attachment #8558671 -
Flags: review?(till) → review+
Assignee | ||
Comment 8•9 years ago
|
||
Thanks for the review comments! The toType code was dating from an age where I would be reusing the return value of ToNumber at the end of the function... remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e6d385cdaa remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a9a074f02927
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f7e6d385cdaa https://hg.mozilla.org/mozilla-central/rev/a9a074f02927
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox38:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•