Closed Bug 1059529 Opened 6 years ago Closed 6 years ago

SIMD backend: implement bitwise operations

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: ivan, Assigned: ivan)

References

Details

Attachments

(1 file, 4 obsolete files)

SIMD.int32x4:
int32x4 and(int32x4 a, int32x4 b)
int32x4 or(int32x4 a, int32x4 b)
int32x4 xor(int32x4 a, int32x4 b)
int32x4 not(int32x4 a)

Although we might want to move the implementation of 'not' to 1044256 because it's a unary operator. I will post an update on this with the WIP patch
Attached patch bug1059529.patch (obsolete) — Splinter Review
This does not have the 'neg' operation implemented, only the 3 others as they are all binary operations.

I didn't have time to confirm that it is correct to remove the existing functions xorps_rr and andps_rr that were mapping to OP2_XORPD_VpdWpd and OP2_ANDPD_VpdWpd.
Attachment #8480280 - Flags: review?(benj)
Rebase before the shuffle patches. Fixes some trailing white space.

The change of 'orps_rr and andps_rr that were mapping to OP2_XORPD_VpdWpd and OP2_ANDPD_VpdWpd' did not cause any jit-test failures, but looks like it needs more scrutiny.
Attachment #8480280 - Attachment is obsolete: true
Attachment #8480280 - Flags: review?(benj)
Attachment #8480414 - Flags: review?(benj)
Rebase before bug 1044256 (the unary maths operations) as this is needed next.
Attachment #8480414 - Attachment is obsolete: true
Attachment #8480414 - Flags: review?(benj)
Attachment #8480471 - Flags: review?(benj)
Comment on attachment 8480471 [details] [diff] [review]
SIMD backend: implement bitwise operations.

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

As I'll probably touch the code, clearing the review flag for now (if i change it, i'll ask review to sunfish); these are mainly notes to myself.

::: js/src/jit/LIR-Common.h
@@ +273,5 @@
> +    LSimdBinaryBitwise() {}
> +
> +  public:
> +    const LAllocation *lhs() {
> +            return getOperand(0);

nit: indent

@@ +284,5 @@
> +    }
> +};
> +
> +// Binary SIMD bitwise operation between two Int32x4 operands
> +class LSimdBinaryBitwiseIx4 : public LSimdBinaryBitwise

no need for subclassing here

::: js/src/jit/Lowering.cpp
@@ +3771,5 @@
> +    JS_ASSERT(IsSimdType(ins->type()));
> +    if (ins->type() == MIRType_Int32x4) {
> +        LSimdBinaryBitwiseIx4 *add = new(alloc()) LSimdBinaryBitwiseIx4();
> +        return lowerForFPU(add, ins, ins->lhs(), ins->rhs());
> +    }

can be applied to float32x4 as well

::: js/src/jit/shared/Assembler-x86-shared.h
@@ +1683,5 @@
> +    }
> +    void orps(const Operand &src, FloatRegister dest) {
> +        JS_ASSERT(HasSSE2());
> +        switch (src.kind()) {
> +            case Operand::FPREG:

nit: indent on this switch and the next one

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +2309,5 @@
> +    JS_ASSERT(ToFloatRegister(ins->output()) == lhs);
> +
> +    MSimdBinaryBitwise::Operation op = ins->operation();
> +    switch (op){
> +        case MSimdBinaryBitwise::and_:

nit: indent

::: js/src/jit/shared/MacroAssembler-x86-shared.h
@@ +465,5 @@
>      }
>      void convertDoubleToFloat32(FloatRegister src, FloatRegister dest) {
>          cvtsd2ss(src, dest);
>      }
> +    void bitwiseAndInt32x4(const Operand &src, FloatRegister dest) {

nit: this actually uses andps so the name sounds wrong. As it will be used for both x4 types, this should probably be bitwiseAndX4. Same for functions below.
Attachment #8480471 - Flags: review?(benj) → feedback+
Attached patch updated patch (obsolete) — Splinter Review
As I've changed the parts I've reviewed, bouncing review to sunfish.
Attachment #8480471 - Attachment is obsolete: true
Attachment #8480526 - Flags: review?(sunfish)
Comment on attachment 8480526 [details] [diff] [review]
updated patch

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

Yummy bitwise operations :). I'd like to do one more review round with comments addressed.

::: js/src/jit/Lowering.cpp
@@ +3767,5 @@
>  
> +bool
> +LIRGenerator::visitSimdBinaryBitwise(MSimdBinaryBitwise *ins)
> +{
> +    JS_ASSERT(IsSimdType(ins->type()));

Please use MOZ_ASSERT instead of JS_ASSERT for new code.

::: js/src/jit/MIR.h
@@ +1509,5 @@
> +        return AliasSet::None();
> +    }
> +
> +    Operation operation() const { return operation_; }
> +};

Please define a congruentTo function for MSimdBinaryBitwise. It just needs to compare the operation_ fields, and then call binaryCongruentTo.

::: js/src/jit/shared/Assembler-x86-shared.h
@@ +1665,5 @@
>              MOZ_CRASH("unexpected operand kind");
>          }
>      }
> +    void andps(const Operand &src, FloatRegister dest) {
> +        JS_ASSERT(HasSSE2());

Here (and several more places below) too, MOZ_ASSERT.

::: js/src/jit/shared/BaseAssembler-x86-shared.h
@@ +3433,5 @@
>      }
>  
>      void andps_rr(XMMRegisterID src, XMMRegisterID dst)
>      {
> +            spew("andps      %s, %s",

Style: the local style here is four-space indentation, here and more below

@@ +3434,5 @@
>  
>      void andps_rr(XMMRegisterID src, XMMRegisterID dst)
>      {
> +            spew("andps      %s, %s",
> +                     nameFPReg(src), nameFPReg(dst));

and arguments should be aligned to the column after the opening ( for the call.

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +2314,5 @@
> +    Operand rhs = ToOperand(ins->rhs());
> +    JS_ASSERT(ToFloatRegister(ins->output()) == lhs);
> +
> +    MSimdBinaryBitwise::Operation op = ins->operation();
> +    switch (op){

Style nit: put a space between ) and {.

::: js/src/jit/shared/MacroAssembler-x86-shared.h
@@ +467,5 @@
>          cvtsd2ss(src, dest);
>      }
>  
> +    void bitwiseAndX4(const Operand &src, FloatRegister dest) {
> +        andps(src, dest);

Using the "ps" variant for all types incurs a domain crossing penalty for integer types and double (when we get there). I think it's fine for now, but please mention this in a comment.
Attachment #8480526 - Flags: review?(sunfish)
Address reviewer feedback.
Attachment #8480526 - Attachment is obsolete: true
Attachment #8481197 - Flags: review?(sunfish)
Comment on attachment 8481197 [details] [diff] [review]
Implement bitwise operations

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

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