Closed Bug 1123777 Opened 5 years ago Closed 5 years ago

SIMD splat and with* functions should coerce their arguments

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: sunfish, Assigned: bbouvier)

Details

Attachments

(2 files, 1 obsolete file)

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.
Looks reasonable to me.  I need to check the behavior in Odin as well.
Assignee: nobody → benj
Status: NEW → ASSIGNED
Flags: needinfo?(benj)
Also factors out the with tests.  And adds tests for splat, which were not
existing (!).
Attachment #8558671 - Flags: review?(till)
Attachment #8558670 - Attachment is obsolete: true
Attachment #8558670 - Flags: review?(luke)
Attached patch asmjs-with.patchSplinter Review
Seems that bzexport only works with mq, not named branches.
Flags: needinfo?(benj)
Attachment #8558672 - Flags: review?(luke)
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+
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/f7e6d385cdaa
https://hg.mozilla.org/mozilla-central/rev/a9a074f02927
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.