811314 - BaselineCompiler: Implement Bitwise ops

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Tom S [:evilpie]]

Attachment: WIP

- I did some unrelated change that are going to be taken out of this patch.
- It's reuse the current ICBinaryArith_Fallback
- Because the shift ops need some special behavior, so I implemented ICShift_Int32
- For the bitwise/shift ops, we should actually add a version for dobules which does truncation.

Comment 2

[Tom S [:evilpie]]

Attachment: WIP v2

Now includes an extra version for shifts where we allow double results.
x86 code - untested, currently on try. https://tbpl.mozilla.org/?tree=Try&rev=ab2eac88bf9b

Comment 3

[Tom S [:evilpie]]

Attachment: v1

Looks reasonable on try now. https://tbpl.mozilla.org/?tree=Try&rev=d783791b06f1

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 681579 [details] [diff] [review]
v1

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

(Discussed some changes on IRC.)

Comment 5

[Tom S [:evilpie]]

Attachment: v2

This patch now aggressively shares the stub code between x86 and x64. I removed the Shift stub type, because it wasn't really necessary. I think we could actually also share the compare stub.

Comment 6

[Tom S [:evilpie]]

Attachment: v2.1

This is now using ExtractTemp1/ExtractTemp2 and ScratchRegIC to abstract between platform differences.

Comment 7

[Tom S [:evilpie]]

Attachment: v3

The "go back to start" version.
I split up x86 and x64 again, because this way it's a lot easier to optimize for different stuff on these two platforms.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 682587 [details] [diff] [review]
v3

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

Awesome, thanks for trying various approaches last week. r=me with comments addressed.

This still needs ARM support, please discuss with djvj if it's okay to do that as a follow-up bug.

::: js/src/ion/BaselineCompiler.cpp
@@ +464,5 @@
> +    return emitBitwise();
> +}
> +
> +bool
> +BaselineCompiler::emitBitwise()

Nit: rename this to emitBinaryArith and call it from emit_JSOP_ADD too.

::: js/src/ion/BaselineIC.h
@@ +582,3 @@
>        protected:
> +        JSOp op;
> +        bool allowDouble;

Nit: op_, allowDouble_

::: js/src/ion/x64/BaselineIC-x64.cpp
@@ +107,5 @@
> +        break;
> +      case JSOP_URSH:
> +        if (!allowDouble) {
> +            masm.movq(R0.valueReg(), ScratchRegIC);
> +        }

Nit: no braces

@@ +119,5 @@
> +            Label toUint;
> +            masm.j(Assembler::Signed, &toUint);
> +
> +            masm.boxValue(JSVAL_TYPE_INT32, ScratchReg, R0);
> +            masm.ret();

Nit: use EmitReturnFromIC here and below. On x64 it doesn't make a difference but good for consistency.

@@ +126,5 @@
> +            masm.convertUInt32ToDouble(ScratchReg, ScratchFloatReg);
> +            masm.boxDouble(ScratchFloatReg, R0);
> +        } else {
> +            masm.j(Assembler::Signed, &revertRegister);
> +

Nit: remove whitespace here.

::: js/src/ion/x86/BaselineIC-x86.cpp
@@ +84,5 @@
> +        break;
> +      case JSOP_BITOR:
> +        // We can overide R0, because the instruction is unfailable.
> +        // The R0.typeReg() is also still intact.
> +        masm.orl(Operand(R1.payloadReg()), R0.payloadReg());

Nit: no need to create an Operand here and two cases below.

@@ +108,5 @@
> +        break;
> +      case JSOP_URSH:
> +        if (!allowDouble) {
> +            masm.movl(R0.payloadReg(), ScratchRegIC);
> +        }

Nit: no braces

@@ +117,5 @@
> +            Label toUint;
> +            masm.j(Assembler::Signed, &toUint);
> +
> +            masm.boxValue(JSVAL_TYPE_INT32, R0.payloadReg(), R0);
> +            masm.ret();

Nit: EmitReturnFromIC here and below.

@@ +124,5 @@
> +            masm.convertUInt32ToDouble(R0.payloadReg(), ScratchFloatReg);
> +            masm.boxDouble(ScratchFloatReg, R0);
> +        } else {
> +            masm.j(Assembler::Signed, &revertRegister);
> +

Nit: remove whitespace here.

::: js/src/ion/x86/BaselineRegisters-x86.h
@@ +28,2 @@
>  
> +static const Register ScratchRegIC = BaselineTailCallReg;

We can't use this register in a stub that calls into the VM, so to avoid confusion I think it's better to be explicit about it:

Register scratch = BaselineTailCallReg in the stub itself

::: js/src/ion/x86/MacroAssembler-x86.h
@@ +159,5 @@
>      }
>      void loadValue(const BaseIndex &src, ValueOperand val) {
>          loadValue(Operand(src), val);
>      }
> +    void boxValue(JSValueType type, Register payload, ValueOperand dest) {

Please use tagValue for now and land a patch to rename it on mozilla-inbound first. I want to avoid having to track down what happened to this method when doing a merge :)

Comment 9

[Tom S [:evilpie]]

Attachment: v4

One last time. Had to fix a small bug with shifts on x64. Kannan could you have a look at the arm stuff?

Comment 10

[Kannan Vijayan [:djvj]]

Comment on attachment 684035 [details] [diff] [review]
v4

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

::: js/src/ion/BaselineIC.cpp
@@ +296,5 @@
> +        if (!UrshOperation(cx, script, stub->icEntry()->pc(script), lhs, rhs, ret.address()))
> +            return false;
> +        break;
> +      }
> +       default:

Nit: indentation on default should be back by one space.

::: js/src/ion/BaselineIC.h
@@ +612,5 @@
> +        // Stub keys shift-stubs need to encode the kind, the JSOp and if we allow doubles.
> +        virtual int32_t getKey() const {
> +            return (static_cast<int32_t>(allowDouble_) | (static_cast<int32_t>(kind) << 1) |
> +                    (static_cast<int32_t>(op_) << 17));
> +        }

Minor change here: let's keep the key definition stable.  This would be better as (kind | op << 16 | allowDouble << 24).

All other keys have kind() in the low 16 bits, and op (if present) in the next 8 bits.

Changing the composition of the key can allow situations where there are inadvertant key conflicts - two different keys that end up composing to the same value.

For example, in this case if the (kind << 1) is also a valid kind (let's say KIND2), and (op << 1) is another valid op (let's say OP2), then the encoding of this key with a false allowDouble will look the same as an encoding of a regular key combining just KIND2 and OP2, namely:

0 | (kind << 1) | (op << (16+1)) == KIND2 | (OP2 << 16)

If we end up having a stub which is keyed on KIND2 and OP2 later on, the stub key will collide with this one.

Comment 11

[Kannan Vijayan [:djvj]]

Comment on attachment 684035 [details] [diff] [review]
v4

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

When running on ARM with some simple test programs the results are wrong, so I'm looking into that right now.

::: js/src/ion/arm/BaselineIC-arm.cpp
@@ +70,5 @@
>  
>      // Add R0 and R1.  Don't need to explicitly unbox, just use R2's payloadReg.
>      Register scratchReg = R2.payloadReg();
>  
>      switch(op) {

Should be op_

@@ +89,5 @@
> +      case JSOP_BITXOR:
> +        masm.ma_eor(R1.payloadReg(), R0.payloadReg(), R0.payloadReg());
> +        break;
> +      case JSOP_BITAND:
> +        masm.ma_and(R1.payloadReg(), R0.payloadReg(), R0.payloadReg();

missing closing paren

@@ +102,5 @@
> +        masm.ma_asr(R1.payloadReg(), R0.payloadReg(), R0.payloadReg());
> +      case JSOP_URSH:
> +        masm.ma_and(Imm32(0x1F), R1.payloadReg(), scratchReg);
> +        masm.ma_lsr(scratchReg, R0.payloadReg(), scratchReg);
> +        masm.ma_cmp(dest, Imm32(0));

I assume you meant scratchReg here instead of dest.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 684035 [details] [diff] [review]
v4

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

Nice! r=me with djvj's and my comments addressed + the ARM-bug (comment 11) fixed.

::: js/src/ion/x86/BaselineIC-x86.cpp
@@ +83,5 @@
> +        break;
> +      case JSOP_BITOR:
> +        // We can overide R0, because the instruction is unfailable.
> +        // The R0.typeReg() is also still intact.
> +        masm.orl(Operand(R1.payloadReg()), R0.payloadReg());

Nit: no need to use Operand(...) here and for the XOR/AND cases below.

Comment 13

[Tom S [:evilpie]]

Thank you both!
http://hg.mozilla.org/projects/ionmonkey/rev/492d0e164f6a