If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla35

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

Trunk
mozilla35
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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
(Assignee)

Comment 1

3 years ago
Created attachment 8495392 [details] [diff] [review]
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).
Attachment #8495392 - Flags: review?(sunfish)
(Assignee)

Comment 2

3 years ago
Created attachment 8495393 [details] [diff] [review]
SIMD: Add int32x4.shift{left,right,rightLogical} to asm.js

With the last refactorings, adding an operation to odin feels even easier.
Attachment #8495393 - Flags: review?(luke)

Comment 3

3 years ago
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)
(Assignee)

Comment 5

3 years ago
(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)

Comment 6

3 years ago
Yeah, explicit is fine and we do that in many other FunctionCompiler method calls.
Flags: needinfo?(luke)
(Assignee)

Updated

3 years ago
Blocks: 1068028
(Assignee)

Comment 7

3 years ago
Created attachment 8495992 [details] [diff] [review]
1) SimdShift

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+
(Assignee)

Comment 9

3 years ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e6e7586b3e02
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4baa041973f8
https://hg.mozilla.org/mozilla-central/rev/e6e7586b3e02
https://hg.mozilla.org/mozilla-central/rev/4baa041973f8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.