Closed Bug 1083238 Opened 5 years ago Closed 5 years ago

SIMD (interpreter): Update shuffle implementations

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(2 files)

[0] seems to make its way to being the next standard, so let's update our interpreter implementation before adding it to ion/odin.

[0] https://github.com/johnmccutchan/ecmascript_simd/issues/74
Till, my review roulette chose you but feel free to bounce it to anybody else.

This does the following:
- renames shuffle => swizzle, shuffleMix => shuffle (see link in comment 0)
- reimplements these functions, giving each of them their own C++ function (fixes a few issues like the fact that shuffleMix behaved as shuffle if it were given 2 arguments rather than 3).
- removes {int,float}32x4shuffle{,mix}.js test files, move all tests in swizzle-shuffle.js and tests everything there (plus more).
Attachment #8505541 - Flags: review?(till)
As stated in the link in comment 0, it doesn't make sense to keep them now that the API has changed anyways. That will please people worrying about memory :)
Attachment #8505546 - Flags: review?(till)
Blocks: 1021716
Comment on attachment 8505541 [details] [diff] [review]
1) Swizzle - Shuffle

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

r=me with nits addressed. (And sorry for the test nits.)

::: js/src/builtin/SIMD.cpp
@@ +975,5 @@
>  {
>      typedef typename V::Elem Elem;
>  
>      CallArgs args = CallArgsFromVp(argc, vp);
> +    if (args.length() != (V::lanes + 1) || !IsVectorObject<V>(args[0]))

I'd switch these, because the second check is the more fundamental one.

@@ +1004,5 @@
> +{
> +    typedef typename V::Elem Elem;
> +
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    if (args.length() != (V::lanes + 2) || !IsVectorObject<V>(args[0]) || !IsVectorObject<V>(args[1]))

Same as above: please move the first check to the end.

::: js/src/tests/ecma_6/TypedObject/simd/swizzle-shuffle.js
@@ +1,5 @@
> +// |reftest| skip-if(!this.hasOwnProperty("SIMD"))
> +
> +/*
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/licenses/publicdomain/

This license is deprecated, please use https://creativecommons.org/publicdomain/zero/1.0/.

@@ +26,5 @@
> +function makeBadArgsForTypeTester(func, type) {
> +    return function() {
> +        var caught = false;
> +        try {
> +            func.apply(type, Array.prototype.slice.call(arguments));

Why do you need the slice here? Doesn't passing in arguments work? If it does, please do that.

@@ +31,5 @@
> +        } catch(e) {
> +            print(e);
> +            caught = true;
> +        }
> +        assertEq(caught, true, 'an exception should have been thrown');

You can use assertThrowsInstanceOf and pass in a closure. That also lets you check the type of the exception to make sure that you don't get some other error.

@@ +42,5 @@
> +        makeBadArgsForTypeTester(function() {}, this)();
> +    } catch (exec) {
> +        makeBadArgsWorks = true;
> +    }
> +    assertEq(makeBadArgsWorks, true);

Same here: use assertThrowsInstanceOf.

@@ +49,5 @@
> +function testSwizzleForType(type) {
> +    var v = type(1,2,3,4);
> +
> +    var tester = makeBadArgsForTypeTester(type.swizzle, type);
> +    tester(v);

Actually, I'm not sure the makeBadArgsForTypeTester is a good thing overall. Just using assertThrowsInstanceOf for all these would be more explicit:

assertThrowsInstanceOf(()=>type.swizzle(v, 0), TypeError);

@@ +57,5 @@
> +    tester(v, 0, 1, 2, 4);
> +    tester(v, 0, 1, 2, -1);
> +    tester(0, 1, 2, 3, v);
> +
> +    // Test all possible swizzles

uber-nit: dot at the end here and below.

@@ +79,5 @@
> +    };
> +    tester(v, 0, 1, 2, obj);
> +
> +    if (typeof reportCompare === "function")
> +        reportCompare(true, true);

You only need this once at the very end of the file, outside of all functions.

@@ +87,5 @@
> +    var v = int32x4(1, 2, 3, 4);
> +
> +    assertThrowsInstanceOf(function() {
> +        float32x4.swizzle(v, 0, 0, 0, 0);
> +    }, Error);

Doesn't this throw a TypeError? Would be good to test for that specifically.

@@ +97,5 @@
> +    var v = float32x4(1, 2, 3, 4);
> +
> +    assertThrowsInstanceOf(function() {
> +        int32x4.swizzle(v, 0, 0, 0, 0);
> +    }, Error);

Same here.

@@ +147,5 @@
> +    };
> +    tester(lhs, rhs, 0, 1, 2, obj);
> +
> +    if (typeof reportCompare === "function")
> +        reportCompare(true, true);

See above.
Attachment #8505541 - Flags: review?(till) → review+
Comment on attachment 8505546 [details] [diff] [review]
2) Remove SIMD shuffle masks

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

Absolutely
Attachment #8505546 - Flags: review?(till) → review+
(In reply to Till Schneidereit [:till] from comment #3)
> Comment on attachment 8505541 [details] [diff] [review]
> 1) Swizzle - Shuffle
> 
> Review of attachment 8505541 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +87,5 @@
> > +    var v = int32x4(1, 2, 3, 4);
> > +
> > +    assertThrowsInstanceOf(function() {
> > +        float32x4.swizzle(v, 0, 0, 0, 0);
> > +    }, Error);
> 
> Doesn't this throw a TypeError? Would be good to test for that specifically.

Thanks for the review! Actually, none of these raise a TypeError, it's just a plain old Error. Should I convert this error to a TypeError, by the way?
Flags: needinfo?(till)
And actually till was on IRC and said yes.
Flags: needinfo?(till)
(In reply to Till Schneidereit [:till] from comment #3)
> Comment on attachment 8505541 [details] [diff] [review]
> 1) Swizzle - Shuffle
> 
> Review of attachment 8505541 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: js/src/builtin/SIMD.cpp
> @@ +975,5 @@
> >  {
> >      typedef typename V::Elem Elem;
> >  
> >      CallArgs args = CallArgsFromVp(argc, vp);
> > +    if (args.length() != (V::lanes + 1) || !IsVectorObject<V>(args[0]))
> 
> I'd switch these, because the second check is the more fundamental one.
Btw, here and below, couldn't do this because the second condition depends on the first one being true (otherwise there might just be no argument).

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3f67c9c3fef6
(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> Btw, here and below, couldn't do this because the second condition depends
> on the first one being true (otherwise there might just be no argument).

D'oh, of course. I was thinking of args.get(index), which returns `undefined` if args.length < index - 1, but yes, not worth it just for having the checks in a slightly more logical order.
https://hg.mozilla.org/mozilla-central/rev/5e32e458318f
https://hg.mozilla.org/mozilla-central/rev/ce11ac061a1b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.