Closed Bug 1043337 Opened 6 years ago Closed 5 years ago

SIMD backend: Implement binary arithmetic operations

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(3 files, 4 obsolete files)

Attached patch Add - WIP (obsolete) — Splinter Review
WIP patch contains code for adds. I plan to add [pun unintended] other operations very soon.
Blocks: 1023404
Attached patch Part 1: SimdBinaryArith and Add (obsolete) — Splinter Review
So this is how I intent to implement it, for realz.
Attachment #8461631 - Flags: feedback?(sunfish)
Attached patch 1. Framework and add (obsolete) — Splinter Review
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 )
Attachment #8461470 - Attachment is obsolete: true
Attachment #8461631 - Attachment is obsolete: true
Attachment #8461631 - Flags: feedback?(sunfish)
Attachment #8461653 - Flags: review?(sunfish)
Attached patch 2. Other arithmetic operations (obsolete) — Splinter Review
Other operations. Feel free to transform into a f? or cancel the review if the previous patch got r-
Attachment #8461655 - Flags: review?(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.
Attachment #8461653 - Flags: review?(sunfish) → review+
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 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).
Attachment #8461655 - Flags: review?(sunfish) → review+
SIMD.float32x4 min and max functions can also be added to this patch's operations
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
Attachment #8461653 - Attachment is obsolete: true
Attachment #8463932 - Flags: review+
Carrying forward r+ from sunfish
Attachment #8463933 - Flags: review+
Attachment #8461655 - Attachment is obsolete: true
Attached patch 3. Min/MaxSplinter Review
With a big comment in CodeGenerator-x86-shared, and a link to bug 1068028.
Attachment #8490129 - Flags: review?(sunfish)
Blocks: 1068096
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.
Attachment #8490129 - Flags: review?(sunfish) → review+
Modified the comment and used directly minps and maxps, thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b8f53b7d329
Keywords: leave-open
Summary: SIMD backend: Implement arithmetic operations → SIMD backend: Implement binary arithmetic operations
https://hg.mozilla.org/mozilla-central/rev/5b8f53b7d329
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Blocks: 1068028
No longer blocks: 1068028
You need to log in before you can comment on or make changes to this bug.