1059529 - SIMD backend: implement bitwise operations

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Ivan Jibaja]

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

Comment 1

[Ivan Jibaja]

Attachment: bug1059529.patch

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.

Comment 2

[Douglas Crosher [:dougc]]

Attachment: SIMD backend: implement bitwise operations

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.

Comment 3

[Douglas Crosher [:dougc]]

Attachment: SIMD backend: implement bitwise operations.

Rebase before bug 1044256 (the unary maths operations) as this is needed next.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

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.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: updated patch

As I've changed the parts I've reviewed, bouncing review to sunfish.

Comment 6

[Dan Gohman [: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.

Comment 7

[Douglas Crosher [:dougc]]

Attachment: Implement bitwise operations

Address reviewer feedback.

Comment 8

[Dan Gohman [:sunfish]]

Comment on attachment 8481197 [details] [diff] [review]
Implement bitwise operations

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

Looks good, thanks!

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/393c2341a26b

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/393c2341a26b