1059408 - OdinMonkey - SIMD: add support for swizzle, shuffle

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: odin-shuffle.patch

Patch to apply atop of those in the dependent bugs

Comment 1

[Dan Gohman [:sunfish]]

Note that there's general agreement to modify the spec here to support a fully general two-input shuffle instead of the current shuffleMix. See [0] for my proposed realization of this idea.

[0] https://github.com/johnmccutchan/ecmascript_simd/issues/74

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Now looking at swizzle/general shuffle. A few questions:
- would it make sense that Odin only accepts constant lane indexes? Or do we need variable lane indexes as well?
- how should we behave if a constant or variable lane index is out of range? clamp it in [0, 4] for swizzle, [0, 8] for shuffle?

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: odin-shuffle.patch

Rebased, but using shuffle/shuffleMix, so not flagging for review yet.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

(note that in comment 2, ranges should be [0, 3] and [0, 7], obviously)

Comment 5

[Dan Gohman [:sunfish]]

I think asm.js can require constant swizzle masks and constant shuffle lane indices. And then it can also require them to be in bounds :-).

Variable values are something that non-asm.js will have to deal with. There's an open question in issue #74 about what to do about out-of-range values. I'd kind of prefer a wraparound to a clamp, but I don't have a strong opinion yet.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Odin SIMD: Add support for swizzle and shuffle

This adds support for swizzle and shuffle. As these functions take 4 literal
integers, I've extracted the logic out of CheckSimdCallArgs to avoid emitting
some unused MConstant for each lane selector, which would be thrown away by GVN
later anyways.

In tests, this patch tests all possible combination of selector lanes, so that
all specific optimized cases get covered as well. Logging shows it takes 5
seconds locally, which is pretty big considering testSIMD ran in 2 seconds
beforehands. Is this ok? Alternatives are to test a few generic cases and edge
cases, but that seems more prone to error. Otherwise, I could split the cases
in testSIMD-shuffle.js in asm.js, or even make a subdir with simd/ test cases
(and start splitting test cases which are spec-only from the ones which are
impl-dependent).

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8505643 [details] [diff] [review]
Odin SIMD: Add support for swizzle and shuffle

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

::: js/src/jit-test/tests/asm.js/testSIMD.js
@@ +870,5 @@
> +            (lanes[3] < 4 ? lhs : rhs)[lanes[3] % 4]];
> +}
> +
> +before = Date.now();
> +for (var i = 0; i < Math.pow(4, 4); i++) {

this should be Math.pow(8, 8), which makes the test way longer :( I'll update tests so as to run only interesting / representative cases.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8505643 [details] [diff] [review]
Odin SIMD: Add support for swizzle and shuffle

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

Very nice!

::: js/src/asmjs/AsmJSValidate.cpp
@@ +5126,5 @@
> +{
> +    for (unsigned i = 0; i < 4; i++, lane = NextNode(lane)) {
> +        uint32_t u32;
> +        if (!IsLiteralInt(f.m(), lane, &u32))
> +            return f.failf(lane, "lane selector should be a constant int32 literal");

I'd say "integer literal" instead of "int32" (since IsLiteralInt accepts BigUnsigned).

@@ +5168,5 @@
> +
> +    ParseNode *arg = CallArgList(call);
> +    MDefinition *vecs[2];
> +
> +    for (unsigned i = 0; i < 2; i++, arg = NextNode(arg)) {

Can you put the \n before 'vecs', not after (or you can remove it altogether)?

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Try was green per bug 1021716 comment 62:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e3bacf8cebd

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e3bacf8cebd