818115 - BaselineCompiler: Compile DIV and MOD

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

Attachment: v1

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

Comment 1

[Jan de Mooij [:jandem]]

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.

Comment 2

[Jan de Mooij [:jandem]]

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;
}

Comment 3

[Tom S [:evilpie]]

Attachment: inline code for x86/x64

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 4

[Jan de Mooij [:jandem]]

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.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: ARM version

Just like the Ion implementation.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: ARM version

Comment 7

[Tom S [:evilpie]]

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

Comment 8

[Jan de Mooij [:jandem]]

ARM version + removed the #ifdef:

https://hg.mozilla.org/projects/ionmonkey/rev/ce883e68337c