- 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.

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

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

Comment on attachment 681579 [details] [diff] [review] v1 Review of attachment 681579 [details] [diff] [review]: ----------------------------------------------------------------- (Discussed some changes on IRC.)

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.

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

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 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 :)

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

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 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 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.

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