This is just the fallback version. The x64 version I am working on right now, sefaults on 3d-raytrace.

Comment on attachment 688308 [details] [diff] [review] v1 Review of attachment 688308 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. The 3d-raytrace crash looked unrelated, I can fix after you finish the patch.

This patch will assert when attaching an Int32 stub, you should just return early before we attach the stub: if (op == JSOP_MOD || op == JSOP_DIV) { // TODO: support optimized DIV/MOD stubs. return true; }

Sorry I forgot to change that, because I was already working on this. No ARM support this time, this is to complex to model without testing. I put quite a lot of comments into this, so it should be understandable.

Comment on attachment 688822 [details] [diff] [review] inline code for x86/x64 Review of attachment 688822 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! r=me with nits addressed. I can add the ARM implementation tomorrow, so either land this patch and ensure we don't attach DIV/MOD stubs on ARM + file a follow-up bug, or wait until the ARM version is ready and I can land both. ::: js/src/ion/x64/BaselineIC-x64.cpp @@ +81,5 @@ > > masm.boxValue(JSVAL_TYPE_INT32, ExtractTemp0, R0.valueReg()); > break; > + case JSOP_DIV: > + masm.unboxInt32(R0, eax); Nit: add the following before this line: JS_ASSERT(R2.scratchReg() == rax); JS_ASSERT(R0.valueReg() != rdx); JS_ASSERT(R1.valueReg() != rdx); @@ +84,5 @@ > + case JSOP_DIV: > + masm.unboxInt32(R0, eax); > + masm.unboxInt32(R1, ExtractTemp0); > + > + // Prevent devision by 0. Nit: division @@ +101,5 @@ > + masm.boxValue(JSVAL_TYPE_INT32, eax, R0.valueReg()); > + break; > + case JSOP_MOD: > + { > + masm.unboxInt32(R0, eax); Nit: add these asserts here too. ::: js/src/ion/x86/BaselineIC-x86.cpp @@ +88,5 @@ > > masm.movl(scratchReg, R0.payloadReg()); > break; > + case JSOP_DIV: > + // Prevent devision by 0. Nit: s/devision/division. @@ +95,5 @@ > + // Prevent negative 0 and -2147483648 / -1. > + masm.branchTest32(Assembler::Zero, R0.payloadReg(), Imm32(0x7fffffff), &failure); > + > + // For idiv we need eax. > + masm.movl(R0.payloadReg(), eax); Add a JS_ASSERT(R1.typeReg() == eax); before this line, it also makes it easier to understand what's going on. @@ +105,5 @@ > + > + // A remainder implies a double result. > + masm.branchTest32(Assembler::NonZero, edx, edx, &revertRegister); > + > + masm.tagValue(JSVAL_TYPE_INT32, eax, R0); masm.movl(eax, R0.payloadReg()); @@ +115,5 @@ > + > + // Prevent negative 0 and -2147483648 % -1. > + masm.branchTest32(Assembler::Zero, R0.payloadReg(), Imm32(0x7fffffff), &failure); > + > + // For idiv we need eax. Nit: JS_ASSERT(R1.typeReg() == eax); @@ +205,5 @@ > + case JSOP_DIV: > + case JSOP_MOD: > + masm.bind(&revertRegister); > + masm.movl(scratchReg, R0.payloadReg()); > + masm.tagValue(JSVAL_TYPE_INT32, ebx, R1); Nit: masm.movl(ImmType(JSVAL_TYPE_INT32), R1.typeReg()); would be a bit clearer, I was wondering which part of R1 we had to restore.

Just like the Ion implementation.

I committed my two patches. http://hg.mozilla.org/projects/ionmonkey/rev/1eec948f6b78 http://hg.mozilla.org/projects/ionmonkey/rev/dbeea6f1da78

ARM version + removed the #ifdef: https://hg.mozilla.org/projects/ionmonkey/rev/ce883e68337c