1252924 - SIMD: Implement toString() and valueOf() on the SIMD.*.prototype

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jakob Stoklund Olesen [:jolesen]]

SIMD objects should support conversion to a string: http://tc39.github.io/ecmascript_simd/#simd-prototype-tostring

The SIMD constructor prototype should also have a valueOf function: http://tc39.github.io/ecmascript_simd/#simd-prototype-valueof

These two methods should be defined directly on the SIMD constructor prototype, otherwise the SIMD polyfill will insert its own.

Comment 1

[Jakob Stoklund Olesen [:jolesen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d7a69cdd5dd

Comment 2

[Jakob Stoklund Olesen [:jolesen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=360fd377edb4

Comment 3

[Jakob Stoklund Olesen [:jolesen]]

Attachment: SIMD.*.prototype.toString()

Reuse the existing toSource() method which happens to produce exactly what the
SIMD.js spec requires.

Comment 4

[Jakob Stoklund Olesen [:jolesen]]

Attachment: SIMD.*.prototype.valueOf.

When SIMD objects have proper value semantics, this method will just return
|this| after a type check.

Until then, do as the polyfill and override valueOf to throw a TypeError so
that ToNumber() can't convert a SIMD value to a number.

Fix wrong code in asm.js/testBug1099216.js. This code was using Float32x4(x)
instead of Float32x4.check(x) as an argument coercion.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8727643 [details] [diff] [review]
SIMD.*.prototype.toString()

Review of attachment 8727643 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't try to find a SIMD.*.prototype.toString spec to verify the spec algorithm is what you say it is.  But it seems plausible enough, and I can't think of what other behavior one might want for it, short of having no toString function at all.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8727643 [details] [diff] [review]
SIMD.*.prototype.toString()

Review of attachment 8727643 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/TypedObject.js
@@ +691,5 @@
>  }
>  
>  function SimdToSource() {
>    if (!IsObject(this) || !ObjectIsTypedObject(this))
> +    ThrowTypeError(JSMSG_INCOMPATIBLE_PROTO, "SIMD.*", "toSource", typeof this);

That said, driveby pre-existing behavior, but |typeof this| here seems doubtful to produce good behavior.  |typeof {}| is "object", but |typeof null| is *also* "object".  I would expect you should be using something other than typeof for this purpose, or at least guarding any typeof with a null-check to handle null differently.  Probably worth fixing, in a different bug.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8727644 [details] [diff] [review]
SIMD.*.prototype.valueOf.

Review of attachment 8727644 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/TypedObject.js
@@ +694,5 @@
> +// Once we have proper value semantics for SIMD types, this function should just
> +// perform a type check and return this.
> +// For now, throw a TypeError unconditionally since valueOf() was probably
> +// called from ToNumber() which is supposed to throw when attempting to convert
> +// a SIMD value to a number.

Okay, guess we might as well keep the way clear.

::: js/src/jit-test/tests/asm.js/testBug1099216.js
@@ +6,5 @@
>  (function(global) {
>      "use asm";
>      var frd = global.Math.fround;
>      var fx4 = global.SIMD.Float32x4;
> +    var fc4 = fx4.check;

I dunno whether the changes in this file are related to this patch or not, but I don't know what Float32x4.check is to say whether this change makes sense or not.

::: js/src/tests/ecma_7/SIMD/toString.js
@@ +13,5 @@
>      assertThrowsInstanceOf(() => ts.call(5), TypeError);
>      assertThrowsInstanceOf(() => ts.call({}), TypeError);
>  
> +    // Can't convert SIMD objects to numbers.
> +    assertThrowsInstanceOf(() => +f, TypeError);

Rather than +f for all of these, why not |f.valueOf()|?  You test toString specifically using a function call, so I don't see why you wouldn't do the same for valueOf.

Comment 8

[Jakob Stoklund Olesen [:jolesen]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> Comment on attachment 8727643 [details] [diff] [review]
> SIMD.*.prototype.toString()
> 
> I didn't try to find a SIMD.*.prototype.toString spec to verify the spec
> algorithm is what you say it is.  But it seems plausible enough, and I can't
> think of what other behavior one might want for it, short of having no
> toString function at all.

Sorry, the spec for toString() is here: <http://tc39.github.io/ecmascript_simd/#simd-prototype-tostring>

The implementation in this patch passes the tests in that repository.

Comment 9

[Jakob Stoklund Olesen [:jolesen]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> Comment on attachment 8727644 [details] [diff] [review]
> SIMD.*.prototype.valueOf.
> 
> ::: js/src/jit-test/tests/asm.js/testBug1099216.js
> @@ +6,5 @@
> >  (function(global) {
> >      "use asm";
> >      var frd = global.Math.fround;
> >      var fx4 = global.SIMD.Float32x4;
> > +    var fc4 = fx4.check;
> 
> I dunno whether the changes in this file are related to this patch or not,
> but I don't know what Float32x4.check is to say whether this change makes
> sense or not.

We don't actually have a spec for SIMD in asm.js yet, but the check() function is how function arguments are annotated:

    function f(n, x) {
        n = n | 0;
        x = fc4(x);
        ...
    }

This indicates that f takes (int, Float32x4) arguments. The semantics or the check() function is simply to either throw a TypeError or return its argument. See <http://tc39.github.io/ecmascript_simd/#simd-check>

The code in this test case looks old, and I think at some point the Float32x4 constructor itself served as the check() function. This test case started failing because Float32x4(x) converts its argument to a number, and it was passed a Float32x4 value. Previously, ToNumber() on a SIMD value would return NaN which was good enough for this test to pass. Now, ToNumber() throws a TypeError, and this test was failing.

> ::: js/src/tests/ecma_7/SIMD/toString.js
> @@ +13,5 @@
> >      assertThrowsInstanceOf(() => ts.call(5), TypeError);
> >      assertThrowsInstanceOf(() => ts.call({}), TypeError);
> >  
> > +    // Can't convert SIMD objects to numbers.
> > +    assertThrowsInstanceOf(() => +f, TypeError);
> 
> Rather than +f for all of these, why not |f.valueOf()|?  You test toString
> specifically using a function call, so I don't see why you wouldn't do the
> same for valueOf.

I am not sure if this counts as a good reason, but the SIMD.js spec requires ToNumber() on a SIMD value to throw, while valueOf() just returns this. See <http://tc39.github.io/ecmascript_simd/#simd-prototype-valueof>. We can't implement that distinction before we have proper value semantics, and I didn't want the tests asserting something that is actually wrong, i.e. the spec requires |+f| to throw a TypeError, but |f.valueOf()| should be |f|.

When we do implement value semantics, we should add something like |assertEq(f, f.valueOf())|.

Comment 10

[Jakob Stoklund Olesen [:jolesen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43ef35e84e99ef2105be6101c4aacf3d7cd94d29
Bug 1252924 - SIMD.*.prototype.toString(); r=waldo

https://hg.mozilla.org/integration/mozilla-inbound/rev/716358ed5dd8f090fb9cda2f55a55bd9cad64fb4
Bug 1252924 - SIMD.*.prototype.valueOf. r=waldo

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/43ef35e84e99
https://hg.mozilla.org/mozilla-central/rev/716358ed5dd8

Comment 12

[Florian Scholz (Open Web Docs)]

Updated reference / added new pages:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD#SIMD_prototype
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/valueOf
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/toString