1043337 - SIMD backend: Implement binary arithmetic operations

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Add - WIP

WIP patch contains code for adds. I plan to add [pun unintended] other operations very soon.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 1: SimdBinaryArith and Add

So this is how I intent to implement it, for realz.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1. Framework and add

Actually, let's make it a r?.

Changes from the previous patches:
- padddw renamed into paddd
- the opcode enum had sd in it for float32x4, although it should ps. (thanks http://ref.x86asm.net/geek.html )

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Other arithmetic operations

Other operations. Feel free to transform into a f? or cancel the review if the previous patch got r-

Comment 4

[Dan Gohman [:sunfish]]

Comment on attachment 8461653 [details] [diff] [review]
1. Framework and add

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

Looks good! I just have a few things to add (back at ya!)...

::: js/src/jit/LIR-Common.h
@@ +237,5 @@
>      SimdLane laneW() const { return laneW_; }
>  };
>  
> +// Binary SIMD arithmetic operation between two SIMD operands
> +class LSimdBinaryArith : public LInstructionHelper<1, 2, 0>

This class could implement the extraName() virtual method to include a string for the operation_ in various spew outputs.

@@ +251,5 @@
> +    const LAllocation *rhs() {
> +        return getOperand(1);
> +    }
> +    MSimdBinaryArith::Operation operation() const {
> +        return operation_;

An alternative to having operation_ as a data member is to just implement this as mir()->operation(). I'm fine with either approach though.

::: js/src/jit/MIR.h
@@ +1330,5 @@
>          return AliasSet::None();
>      }
>  };
>  
> +class MSimdBinaryArith : public MBinaryInstruction

This class can implement congruentTo to enable GVN of redundant simd instructions (though this will be hopefully rare under emscripten). You can use binaryCongruentTo to do most of the work; you just need to supplement with checking operation_.

@@ +1348,5 @@
> +        JS_ASSERT(IsSimdType(type));
> +        JS_ASSERT(left->type() == right->type());
> +        JS_ASSERT(left->type() == type);
> +        setResultType(type);
> +        setMovable();

For Add, we can do setCommutative() too.

Comment 5

[Dan Gohman [:sunfish]]

Comment on attachment 8461653 [details] [diff] [review]
1. Framework and add

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

::: js/src/assembler/assembler/X86Assembler.h
@@ +328,5 @@
>          OP2_XADD_EvGv       = 0xC1,
>          OP2_PEXTRW_GdUdIb   = 0xC5,
>          OP2_SHUFPS_VpsWpsIb = 0xC6,
>          OP2_PXORDQ_VdqWdq   = 0xEF,
> +        OP2_PADDD_VdqWdq   = 0xFE

Please add a space here to keep things aligned.

Comment 6

[Dan Gohman [:sunfish]]

Comment on attachment 8461655 [details] [diff] [review]
2. Other arithmetic operations

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

Currently the polyfill implementation doesn't have int32x4 div, but it does have int32x4 mul. Is there a plan for how this will work?

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +2179,5 @@
> +        masm.packedSubInt32(rhs, lhs);
> +        return true;
> +      case MSimdBinaryArith::Mul:
> +      case MSimdBinaryArith::Div:
> +        break;

Please add a comment here mentioning that x86 doesn't have SIMD i32 div, and that we can do mul with a single instruction only if we have SSE4.1 (PMULLD).

Comment 7

[Ivan Jibaja]

SIMD.float32x4 min and max functions can also be added to this patch's operations

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1. Framework and Add

Thanks for the review. In the LIR instruction, it's ideal to reuse the mir_ and to get the operation from there, as it avoids storing more in the LIR.

Carrying forward r+ from sunfish

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Other arithmetic operations

Carrying forward r+ from sunfish

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c667b20a1d07
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0a61ae6d603f

Min/max in other patches, to not block the MIR.h changes.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c667b20a1d07
https://hg.mozilla.org/mozilla-central/rev/0a61ae6d603f

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3. Min/Max

With a big comment in CodeGenerator-x86-shared, and a link to bug 1068028.

Comment 13

[Dan Gohman [:sunfish]]

Comment on attachment 8490129 [details] [diff] [review]
3. Min/Max

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

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +2370,5 @@
> +        // - what if NaN is one of the operands? The Intel x86 developer manual
> +        // states the return value may be either one of the operand or NaN,
> +        // according to the register in which it is.
> +        // - what about -0 and 0? The Intel x86 developer manual states that
> +        // min/max will return 0 in any cases.

It states that min/max will return the "second operand" in the instruction if both values are zero, or if one is a NaN.

::: js/src/jit/shared/MacroAssembler-x86-shared.h
@@ +542,5 @@
>      }
>      void packedDivFloat32(const Operand &src, FloatRegister dest) {
>          divps(src, dest);
>      }
> +    void packedMaxFloat32(const Operand &src, FloatRegister dest) {

packedMaxFloat32 is a kind of target-independent name, and the fit isn't perfect because x86's maxps is odd. I'd prefer to either
 - extend packedMaxFloat32 to do the full NaN-propagating negative-zero-respecting max
or
 - drop packedMaxFloat32 for now and just call maxps from CodeGen directly.

@@ +545,5 @@
>      }
> +    void packedMaxFloat32(const Operand &src, FloatRegister dest) {
> +        maxps(src, dest);
> +    }
> +    void packedMinFloat32(const Operand &src, FloatRegister dest) {

Likewise.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Modified the comment and used directly minps and maxps, thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b8f53b7d329

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/5b8f53b7d329