Closed Bug 1073064 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(2 files, 1 obsolete file)

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
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).
Attachment #8495392 - Flags: review?(sunfish)
With the last refactorings, adding an operation to odin feels even easier.
Attachment #8495393 - Flags: review?(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.
Attachment #8495393 - Flags: review?(luke) → review+
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?
Attachment #8495392 - Flags: review?(sunfish)
(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).
Flags: needinfo?(luke)
Yeah, explicit is fine and we do that in many other FunctionCompiler method calls.
Flags: needinfo?(luke)
Blocks: 1068028
Attached patch 1) SimdShiftSplinter Review
With comments addressed. As discussed on IRC, i've used packed*ShiftByScalar rather than Uniform.
Attachment #8495392 - Attachment is obsolete: true
Attachment #8495992 - Flags: review?(sunfish)
Comment on attachment 8495992 [details] [diff] [review]
1) SimdShift

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

Looks good, thanks!
Attachment #8495992 - Flags: review?(sunfish) → review+
https://hg.mozilla.org/mozilla-central/rev/e6e7586b3e02
https://hg.mozilla.org/mozilla-central/rev/4baa041973f8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.