Closed
Bug 1083238
Opened 10 years ago
Closed 10 years ago
SIMD (interpreter): Update shuffle implementations
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: bbouvier, Assigned: bbouvier)
References
Details
Attachments
(2 files)
20.61 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
15.33 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
[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
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
(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)
Assignee | ||
Comment 7•10 years ago
|
||
(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
Assignee | ||
Comment 8•10 years ago
|
||
See try build in comment 7 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e32e458318f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ce11ac061a1b
Comment 9•10 years ago
|
||
(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.
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5e32e458318f https://hg.mozilla.org/mozilla-central/rev/ce11ac061a1b
Status: NEW → 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.
Description
•