Closed Bug 1059408 Opened 9 years ago Closed 9 years ago

OdinMonkey - SIMD: add support for swizzle, shuffle


(Core :: JavaScript Engine: JIT, defect)

Not set





(Reporter: bbouvier, Assigned: bbouvier)




(2 files, 1 obsolete file)

Attached patch odin-shuffle.patch (obsolete) — Splinter Review
Patch to apply atop of those in the dependent bugs
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.

Summary: OdinMonkey - SIMD: add support for shuffle, shuffleMix → OdinMonkey - SIMD: add support for swizzle, shuffle
Blocks: 1079362
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?
Flags: needinfo?(sunfish)
Rebased, but using shuffle/shuffleMix, so not flagging for review yet.
Attachment #8479998 - Attachment is obsolete: true
(note that in comment 2, ranges should be [0, 3] and [0, 7], obviously)
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.
Flags: needinfo?(sunfish)
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
Attachment #8505643 - Flags: review?(luke)
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 =;
> +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 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)?
Attachment #8505643 - Flags: review?(luke) → review+
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.