1073064 - SIMD: Implement logical operators (lsh, rhs and ursh) and add support in Odin

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

SIMD.int32x4.shiftLeft(vec, val) => [vec.x << val, vec.y << val, vec.z << val, vec.w << val]

SIMD.int32x4.shiftRight ~rsh
SIMD.int32x4.shiftRightLogical ~ursh

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: SIMD x86-x64 backend: Implement MSimdBinaryLogical

I also wanted to implement float32x4.scale in the same MIR node, as they relate
by having a first vector arg and second scalar arg. Actually, scale has the
same issue as withFlags and zero(): it doesn't map to an x86 instruction and it
can be emulated with other operators (I've added it in the github discussion).

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: SIMD: Add int32x4.shift{left,right,rightLogical} to asm.js

With the last refactorings, adding an operation to odin feels even easier.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8495393 [details] [diff] [review]
SIMD: Add int32x4.shift{left,right,rightLogical} to asm.js

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

::: js/src/asmjs/AsmJSValidate.cpp
@@ +2463,5 @@
>          curBlock_->add(ins);
>          return ins;
>      }
>  
> +    MDefinition *binarySimd(MDefinition *lhs, MDefinition *rhs, MSimdBinaryLogical::Operation op)

I think you could have a:
  template <class T>
  MDefinition *binarySimd(MDefininition *lhs, MDefinition *rhs, T::Operation op, MIRType type);
that collapsed this with the Arith and Bitwise overloads.

Comment 4

[Dan Gohman [:sunfish]]

Comment on attachment 8495392 [details] [diff] [review]
SIMD x86-x64 backend: Implement MSimdBinaryLogical

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

::: js/src/jit/LIR-Common.h
@@ +325,5 @@
>          return mir_->toSimdBinaryBitwise()->operation();
>      }
>  };
>  
> +class LSimdBinaryLogical : public LInstructionHelper<1, 2, 0>

Since this class expects its right operand to be an int32, rather than a SIMD value, it's pretty specific to shifting (and maybe also rotating). How about LSimdShift? Or maybe LSimdShiftUniform?

::: js/src/jit/MIR.h
@@ +1717,5 @@
>  
>      ALLOW_CLONE(MSimdBinaryBitwise)
>  };
>  
> +class MSimdBinaryLogical : public MBinaryInstruction

If you rename LSimdBinaryLogical, keep this analogous.

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +2538,5 @@
> +    FloatRegister out = ToFloatRegister(ins->output());
> +    MOZ_ASSERT(vec == out); // defineReuseInput(0);
> +
> +    FloatRegister tmp = ScratchFloat32Reg;
> +    masm.movd(val, tmp);

Shifting by constant is likely to be our main use case, since ARM appears to lack a non-immediate shift. On Intel, shift-by-immediate is 1 clock, while shifting by a dynamic value is 2 clocks plus 1 for the movd plus 1 to put the count in a gpr for the movd. Consequently, I think it's important to optimize the case where the shift count is a constant now rather than later. It should just be a useRegisterOrConstant in the lowering code, assembler support for the immediate variants, and code here to handle the immediate case separate from the dynamic case.

There's also the question here of what to do when the shift is out of range. I filed this:
https://github.com/johnmccutchan/ecmascript_simd/issues/72
and I think just keeping the x86 behavior here for now is fine.

::: js/src/jit/shared/MacroAssembler-x86-shared.h
@@ +524,5 @@
>      void packedSubInt32(const Operand &src, FloatRegister dest) {
>          psubd(src, dest);
>      }
>  
> +    void packedLeftShift(FloatRegister src, FloatRegister dest) {

These names are a little ambiguous given that they shift all elements of dest by the count in the *first* element of src, in contrast to e.g. vpsllvd which shifts elements of dest by the counts in the *corresponding* elements of src (and is the more intuitive "vectorized" version of a shift, at least to me). How about packedLeftShiftUniform?

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #3)
> Comment on attachment 8495393 [details] [diff] [review]
> SIMD: Add int32x4.shift{left,right,rightLogical} to asm.js
> 
> Review of attachment 8495393 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/asmjs/AsmJSValidate.cpp
> @@ +2463,5 @@
> >          curBlock_->add(ins);
> >          return ins;
> >      }
> >  
> > +    MDefinition *binarySimd(MDefinition *lhs, MDefinition *rhs, MSimdBinaryLogical::Operation op)
> 
> I think you could have a:
>   template <class T>
>   MDefinition *binarySimd(MDefininition *lhs, MDefinition *rhs, T::Operation
> op, MIRType type);
> that collapsed this with the Arith and Bitwise overloads.

Thanks for the review! Unfortunately, T::Operation is a dependent type so the compiler cannot infer which type is that. If you're okay with explicit instanciation of the template, I can do it. Also, this could be collapsed with the SimdBinaryComp overload only, as the signatures with Arith and Bitwise are different (they include a MIRType).

Comment 6

[Luke Wagner [:luke]]

Yeah, explicit is fine and we do that in many other FunctionCompiler method calls.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1) SimdShift

With comments addressed. As discussed on IRC, i've used packed*ShiftByScalar rather than Uniform.

Comment 8

[Dan Gohman [:sunfish]]

Comment on attachment 8495992 [details] [diff] [review]
1) SimdShift

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

Looks good, thanks!

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e6e7586b3e02
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4baa041973f8

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e6e7586b3e02
https://hg.mozilla.org/mozilla-central/rev/4baa041973f8