1060437 - Odin SIMD: Implement the select operation.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Comment 1

[Marty Rosenberg [:mjrosenb]]

Attachment: addSelect-r0.patch

It looks like support for int32x4.select already exists in the interpreter. This should add it in to odin.  I'm a tad fuzzy on x86 codegen, so I just required that all operands be registers, and the dest is separate from the source.  Implementing the float32x4 case should be nearly trivial.

Comment 2

[Douglas Crosher [:dougc]]

Look like the spec. might have changed here. The interpreter, implements Int32x4Select as accepting an Int32x4 selection argument, but float32 value arguments. We might need to fix this up before we can add the float32x4 version that we need?

Comment 3

[Douglas Crosher [:dougc]]

Attachment: quick attempt at float32x4 support

Just a very quick attempt to get float32x4 support going. Lot's of issues I want to look into further, but it seems to work for the Flappy Bird demo.

Comment 4

[Douglas Crosher [:dougc]]

Attachment: Implement the select operation.

The select sequence is not performing well. This patch modifies the lowering register definitions to make all arguments useRegsiterAtStart(), removes the temporary register, defines the output to reuse the same register as the second argument, then optimizes the sequence for this.

Comment 5

[Douglas Crosher [:dougc]]

Attachment: Implement the select operation.

Added float32x4 support, rebased, optimized the register allocation, moved the lowering into the backends as it looks backend specific.

It works for the Flappy Bird demo, but it lacks testing. Not sure about the interpreter support. Not sure about the naming of methods, and the organization.

This is not critical for Flappy Bird, but might be nice to have as it would avoid a hack workaround in the Flappy Bird source code. If you agree then could you please help get this patch cleaned up.

For some reason this function seems to hit performance badly so I had to move it out of the hot path in Flappy Bird.

Comment 6

[Douglas Crosher [:dougc]]

Attachment: Implement the select operation.

Fix the test failures. It seems the spec. changed since the int32x4select.js test was written. There are now separate int32x4 and float32x4 select methods, and they both accept an int32x4 'test' argument. Added an separate test for float32x4.select. Also added some tests for edge cases.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8482056 [details] [diff] [review]
Implement the select operation.

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

Thanks for doing that! Mostly nits and a few renaming issues in my opinion, but that looks good. Ideally, we'd split this patch into three parts, ordered this way:
1) changes in SIMD.h/cpp + ES tests (i can review that)
2) implement the MIR SIMD instruction (sunfish or i can review this one)
3) add support in Odin + asm.js tests (and luke would review this one, to make sure this fits odin's style)
What do you think?

::: js/src/asmjs/AsmJSValidate.cpp
@@ +2434,5 @@
>          curBlock_->add(ins);
>          return ins;
>      }
>  
> +    MDefinition *ternarySimd(MDefinition *lhs, MDefinition *mid, MDefinition *rhs,

How about naming these: mask, lhs, rhs?

@@ +2439,5 @@
> +                             MSimdTernaryBitwise::Operation op, MIRType type)
> +    {
> +        if (inDeadCode())
> +            return nullptr;
> +        JS_ASSERT(IsSimdType(lhs->type()));

JS_ASSERT(lhs->type() == MIRType_Int32x4); // mask->type(), with suggested renaming

@@ +4772,5 @@
>        case AsmJSSimdOperation_xor:
>          *def = f.binarySimd(lhsDef, rhsDef, MSimdBinaryBitwise::xor_, opType);
>          *type = retType;
>          break;
> +      default:

Please replace this default case with case AsmJSSimdOperation_select, so that we get a warning from the compiler when we forget to add a case for another simd operation.

@@ +4785,5 @@
> +                 MDefinition **def, Type *type)
> +{
> +    unsigned numArgs = CallArgListLength(call);
> +    if (numArgs != 3)
> +        return f.failf(call, "expected 2 arguments to binary arithmetic SIMD operation, got %u", numArgs);

nit: expected 3 arguments to ternary SIMD operation, got ...

@@ +4794,5 @@
> +
> +    MDefinition *lhsDef, *midDef, *rhsDef;
> +    Type lhsType, midType, rhsType;
> +    if (!CheckExpr(f, lhs, &lhsDef, &lhsType))
> +        return false;

we should check that lhsType is Type::Int32x4 here, and fail otherwise.

Can you please add tests for this operation to tests/asm.js/testSIMD.js, in the section Operations?

@@ +4805,5 @@
> +    if (midType != retType || rhsType != retType)
> +        return f.failf(lhs, "last two arguments to SIMD ternary op should both be %s", retType.toChars());
> +
> +    MIRType opType = retType.toMIRType();
> +    switch (global->simdOperation()) {

As there's only one operation so far, we could go with an impl with no switch at all

::: js/src/builtin/SIMD.cpp
@@ +757,5 @@
> +    // bits select the first lane, the next log2(L) the second, and so on.
> +    const uint32_t SELECT_SHIFT = FloorLog2(V::lanes);
> +    const uint32_t SELECT_MASK  = V::lanes - 1;
> +    const int32_t MAX_MASK_VALUE = int32_t(pow(double(V::lanes), double(V::lanes))) - 1;
> +    JS_ASSERT(MAX_MASK_VALUE > 0);

Double check me, but I think that's not needed.

@@ +762,5 @@
> +
> +    Elem result[V::lanes];
> +
> +    JS_ASSERT(args.length() == 3);
> +    if (!IsVectorObject<Int32x4>(args[0]) || !IsVectorObject<V>(args[1]) || !IsVectorObject<V>(args[2].isInt32()))

last part of the condition should be !IsVectorObject<V>(args[2]), not args[2].isInt32(). That will certainly fail in testing.

@@ +767,5 @@
> +        return ErrorBadArgs(cx);
> +
> +    Elem *mask = TypedObjectMemory<Elem *>(args[0]);
> +    Elem *val1 = TypedObjectMemory<Elem *>(args[2]);
> +    Elem *val2 = TypedObjectMemory<Elem *>(args[3]);

Something wrong with the array accesses here: args.length() is asserted to be 3, so these two array accesses should be args[1] and args[2], right?

@@ +769,5 @@
> +    Elem *mask = TypedObjectMemory<Elem *>(args[0]);
> +    Elem *val1 = TypedObjectMemory<Elem *>(args[2]);
> +    Elem *val2 = TypedObjectMemory<Elem *>(args[3]);
> +    int32_t maskArg;
> +    if (!ToInt32(cx, args[2], &maskArg))

According to my reading of the polyfill, args[0] is an int32x4, args[1] and args[2] are V, so this operation will certainly fail.

@@ +772,5 @@
> +    int32_t maskArg;
> +    if (!ToInt32(cx, args[2], &maskArg))
> +        return false;
> +    if (maskArg < 0 || maskArg > MAX_MASK_VALUE)
> +        return ErrorBadArgs(cx);

This test should go away.

@@ +779,5 @@
> +    for (; i < V::lanes; i++)
> +        result[i] = mask[i] ? val1[i] : val2[i];
> +
> +    return StoreResult<V>(cx, args, result);
> +}

now that i've reviewed the entire function, I see it's unused. We have two solutions:

1) remove this function, land it with the float32x4/int32x4 variants and factor it out in a follow up bug.
2) use this function and remove int32x4select and float32x4select below.

As we're on a pretty tight schedule, I'm fine with 1, but it's up to you.

@@ +951,5 @@
>          return ErrorBadArgs(cx);
>      }
>  
>      int32_t *val = TypedObjectMemory<int32_t *>(args[0]);
>      int32_t *tv = TypedObjectMemory<int32_t *>(args[1]);

It looks strange that this function stays as is, especially as args[1] and args[2] are now float*. This should work as we're just looking at bits, but the templatized version would be fine too here.

::: js/src/jit/LIR-Common.h
@@ +308,5 @@
>  };
>  
> +// SIMD selection of lanes from two int32x4 or float32x4 arguments based on a
> +// int32x4 argument.
> +class LSimdSelect : public LInstructionHelper<1, 3, 1>

As it's lowered in Lowering-x86-shared.h, I'd be in favor with moving this class in LIR-x86-shared.h as well, and implement stub classes for ARM and MIPS

@@ +312,5 @@
> +class LSimdSelect : public LInstructionHelper<1, 3, 1>
> +{
> +  public:
> +    LIR_HEADER(SimdSelect);
> +    const LAllocation *lhs() {

Could you go with the renaming here, and in other files, as suggested in some other place: mask, lhs, rhs?

@@ +322,5 @@
> +    const LAllocation *rhs() {
> +        return getOperand(2);
> +    }
> +    MSimdBinaryBitwise::Operation operation() const {
> +        return mir_->toSimdBinaryBitwise()->operation();

nit: toSimdTernaryBitwise

::: js/src/jit/MIR.h
@@ +1603,5 @@
> +        MOZ_ASSERT(IsSimdType(type));
> +        // thusfar, the only operation is select, where the result, mid, and rhs are the same
> +        // and the left, which is the selector is always int.
> +        MOZ_ASSERT(mid->type() == right->type());
> +        MOZ_ASSERT(mid->type() == type);

MOZ_ASSERT(lhs->type() == MIRType_Int32x4);

@@ +1621,5 @@
> +        return AliasSet::None();
> +    }
> +
> +    Operation operation() const { return operation_; }
> +

nit: blank line

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +2379,5 @@
> +    FloatRegister onFalse = ToFloatRegister(ins->getOperand(2));
> +    MOZ_ASSERT(onTrue == ToFloatRegister(ins->output()));
> +    MOZ_ASSERT(cond != onTrue);
> +    // The onFalse argument is not destroyed but due to limitations of the
> +    // register allocator it's life ends at the start of the operation.

nit: s/it's/its

::: js/src/jit/shared/Lowering-x86-shared.cpp
@@ +309,5 @@
>  
>  bool
> +LIRGeneratorX86Shared::visitSimdTernaryBitwise(MSimdTernaryBitwise *ins)
> +{
> +    JS_ASSERT(IsSimdType(ins->type()));

please MOZ_ASSERT here

@@ +318,5 @@
> +        lins->setOperand(1, useRegisterAtStart(ins->getOperand(1)));
> +        // This could be useRegister(), but combining it with
> +        // useRegisterAtStart() is broken see bug 772830.
> +        lins->setOperand(2, useRegisterAtStart(ins->getOperand(2)));
> +        return defineReuseInput(lins, ins, 1);

Why do we use defineReuseInput here and not define? (not saying it's wrong, but not sure why we'd constrain more the regalloc here)

::: js/src/tests/ecma_6/TypedObject/simd/float32x4selext.js
@@ +1,1 @@
> +// |reftest| skip-if(!this.hasOwnProperty("SIMD"))

uber-nit: please rename this file to float32x4select.js

Comment 8

[Douglas Crosher [:dougc]]

Attachment: 1. SIMD: Implement the select operation.

Thank you for the feedback.

Deleted the unused offending function, and fixed the file name.

Comment 9

[Douglas Crosher [:dougc]]

Attachment: 2. SIMD backend: Implement the select operation.

Addressed much of the feedback.

The defineReuseInput() usage is required because an input is destroyed and becomes the output, a common pattern on the x86.

Also made use of the blendvps instruction for SSE 4.1. Not tested, but easily disabled if it fails. Hoping this might help performance.

Comment 10

[Douglas Crosher [:dougc]]

Attachment: 3. Odin SIMD: Implement the select operation.

Addressed feedback. Added a few tests.

Note two of the tests are disabled as they cause a dead code assertion failure. Do not have time to look into it now, sorry. Can you spot what is going wrong?

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8482336 [details] [diff] [review]
1. SIMD: Implement the select operation.

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

Perfect, thanks!

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8482337 [details] [diff] [review]
2. SIMD backend: Implement the select operation.

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

Trying to help to figure out what the issue is with the sse4 impl, full review coming later today.

::: js/src/jit/shared/Lowering-x86-shared.cpp
@@ +325,5 @@
> +            lins->setOperand(2, useRegisterAtStart(ins->getOperand(2)));
> +            // The output is constained to be in the same register as the third
> +            // argument to avoid a redundant copy instruction when not
> +            // necessary.
> +            return defineReuseInput(lins, ins, 2);

shouldn't this be defineReuseInput(lins, ins, 1); ?

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8482338 [details] [diff] [review]
3. Odin SIMD: Implement the select operation.

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

Except for the error reporting issue, looks good to me. Thanks!

::: js/src/asmjs/AsmJSValidate.cpp
@@ +3573,5 @@
>                                 ParseNode *initNode, PropertyName *varName, PropertyName *ctorVarName,
>                                 PropertyName *opName)
>  {
>      AsmJSSimdType simdType = global->simdCtorType();
> +

nit: blank line

@@ +4797,5 @@
> +    Type maskType, lhsType, rhsType;
> +    if (!CheckExpr(f, mask, &maskDef, &maskType))
> +        return false;
> +    if (maskType != Type::Int32x4)
> +        return false;

you need to fail explicitly, as there won't be any error reported otherwise:

if (maskType != Type::Int32x4)
  return f.failf(mask, "%s is not a subtype of int32x4", maskType.toChars());

or something like that. After that, you should be able to re-enable your tests. Please flag again for r? if it isn't enough.

Comment 14

[Douglas Crosher [:dougc]]

Attachment: 2. SIMD backend: Implement the select operation.

Comment 15

[Douglas Crosher [:dougc]]

Attachment: 3. Odin SIMD: Implement the select operation.

Address feedback. Carry forward r+.

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8482509 [details] [diff] [review]
2. SIMD backend: Implement the select operation.

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

Thanks!

::: js/src/jit/LIR-Common.h
@@ +308,5 @@
>  };
>  
> +// SIMD selection of lanes from two int32x4 or float32x4 arguments based on a
> +// int32x4 argument.
> +class LSimdSelect : public LInstructionHelper<1, 3, 1>

nit: no need for the temporary here: LInstructionHelper<1, 3, 0> should be enough

::: js/src/jit/MIR.h
@@ +1601,5 @@
> +      : MTernaryInstruction(mask, lhs, rhs), operation_(op)
> +    {
> +        MOZ_ASSERT(IsSimdType(type));
> +        // Thus far, the only operation is select, where the result, mid, and
> +        // rhs are the same and the left, which is the selector is always int.

nit: the comment is stall with new variable names, i think we'd be good deleting it

::: js/src/jit/shared/Lowering-x86-shared.cpp
@@ +314,5 @@
> +
> +    if (ins->type() == MIRType_Int32x4 || ins->type() == MIRType_Float32x4) {
> +        LSimdSelect *lins = new(alloc()) LSimdSelect;
> +
> +        // This must be useRegisterAtStart() because it is destroyed.

Do we need all these comments about the kind of uses?

Comment 17

[Douglas Crosher [:dougc]]

Attachment: 2. SIMD backend: Implement the select operation.

Thank you. Good catch on the temporary, I did think I had removed it. Can I leave the comments related to the register allocation - we can not yet expression the exact requirements, and there is a bug to work around, and the comments might help someone rewrite the code in future.

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: bug1060437-odin.patch

I would like to make sure that this fits Odin's style.

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

First two patches:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d1cbbceeb19e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/974abe2bd949

Comment 20

[Luke Wagner [:luke]]

Comment on attachment 8482585 [details] [diff] [review]
bug1060437-odin.patch

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

::: js/src/asmjs/AsmJSValidate.cpp
@@ +4796,5 @@
> +    MDefinition *maskDef;
> +    Type maskType;
> +    if (!CheckExpr(f, mask, &maskDef, &maskType))
> +        return false;
> +    if (maskType != Type::Int32x4)

Given that this is specialized to select, I'd rename this to CheckSimdSelect.  Alternatively, as suggested in the other patch, if we generalized this to an n-ary CheckSimdCall, two of the arguments could be the expected arity and an array of argument types passed by the caller.

::: js/src/jit-test/tests/asm.js/testSIMD.js
@@ +550,5 @@
> +
> +assertEqX4(asmLink(asmCompile('glob', USE_ASM + I32 + F32 + F32SEL + "function f() {var m=i4(0,0,0,0); var x=f4(1,2,3,4); var y=f4(5,6,7,8); return f4(f4sel(m,x,y)); } return f"), this)(), [5, 6, 7, 8]);
> +assertEqX4(asmLink(asmCompile('glob', USE_ASM + I32 + F32 + F32SEL + "function f() {var m=i4(0xffffffff,0xffffffff,0xffffffff,0xffffffff); var x=f4(1,2,3,4); var y=f4(5,6,7,8); return f4(f4sel(m,x,y)); } return f"), this)(), [1, 2, 3, 4]);
> +assertEqX4(asmLink(asmCompile('glob', USE_ASM + I32 + F32 + F32SEL + "function f() {var m=i4(0,0xffffffff,0,0xffffffff); var x=f4(1,2,3,4); var y=f4(5,6,7,8); return f4(f4sel(m,x,y)); } return f"), this)(), [5, 2, 7, 4]);
> +assertEqX4(asmLink(asmCompile('glob', USE_ASM + I32 + F32 + F32SEL + "function f() {var m=i4(0,0,0xffffffff,0xffffffff); var x=f4(1,2,3,4); var y=f4(5,6,7,8); return f4(f4sel(m,x,y)); } return f"), this)(), [5, 6, 3, 4]);

Could you also add test cases for int/float that have only some of the bits set?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d1cbbceeb19e
https://hg.mozilla.org/mozilla-central/rev/974abe2bd949

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: bug1060437-odin-select.patch

carrying forward r+ from luke.
About the tests: I haven't added them, because the behavior of select isn't well specified if the masks components values aren't only 0xFFFFFFFF or 0x0. Using select on other bit patterns will lead to messy behavior right now (mixing both vectors input). Opened [1] about that topic.

Opened bug 1062217 for generalizing SIMD operation call checking, as I'd like to land this quickly (needed for the FBirds demo).

[1] https://github.com/johnmccutchan/ecmascript_simd/issues/53

Comment 23

[Benjamin Bouvier [:bbouvier] (inactive)]

(Actually, I started to write one test, and forgot to remove it in the previous patch. Done) 

https://hg.mozilla.org/integration/mozilla-inbound/rev/9635de3e6440

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9635de3e6440