848512 - BaselineCompiler: JSOP_DIV: int32 stub should handle lhs == 0 better

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Currently the stub gives up if lhs == 0:

// Prevent negative 0 and -2147483648 / -1.
masm.branchTest32(Assembler::Zero, R0.payloadReg(), Imm32(0x7fffffff), &failure);

It should try a little harder and handle the case where the result is +0. This happens a lot on v8-raytrace, currently we keep attaching int32 stubs until we reach the limit..

Comment 1

[Sankha Narayan Guria [:sankha]]

Attachment: patch v1

Jandem can you see if this is correct? I ran the raytrace script from the v8 benchmark. I am getting the following score (an average of 3 runs):

With patch: 22589
Without patch: 21012

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8359864 [details] [diff] [review]
patch v1

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

Thanks for looking into this! I completely forgot about this bug.

::: js/src/jit/x86/BaselineIC-x86.cpp
@@ +93,5 @@
>          // Prevent negative 0 and -2147483648 / -1.
>          masm.branchTest32(Assembler::Zero, R0.payloadReg(), Imm32(0x7fffffff), &failure);
>  
> +        // Prevent positive 0.
> +        masm.branchTest32(Assembler::Zero, R0.payloadReg(), Imm32(0), &failure);

This branch will never be taken I think. The previous branch handles everything that has the lower 31-bits cleared, including 0.

I think the right fix is to split the previous check: first, if R0 == INT32_MIN, jump to |failures|. Then, if R0 == 0 AND R1 < 0, also jump to failures (result is -0 in this case).

Let me know if you need help with this :)

Comment 3

[Sankha Narayan Guria [:sankha]]

Attachment: patch v2

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8366606 [details] [diff] [review]
patch v2

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

::: js/src/jit/x86/BaselineIC-x86.cpp
@@ +92,5 @@
>  
>          // Prevent negative 0 and -2147483648 / -1.
> +        masm.branchTest32(Assembler::Equal, R0.payloadReg(), Imm32(INT32_MIN), &failure);
> +        masm.branchTest32(Assembler::Equal, R0.payloadReg(), Imm32(0), &failure);
> +        masm.branchTest32(Assembler::Signed, R1.payloadReg(), R1.payloadReg(), &failure);

This jumps to |failure| if R0 == 0 OR R1 < 0. Instead, we want it to jump to |failure| if R0 == 0 AND R1 < 0. So you want something like this:

masm.branchTest32(Assembler::Equal, R0.payloadReg(), Imm32(INT32_MIN), &failure);

Label notZero;
masm.branchTest32(Assembler::NotEqual, R0.payloadReg(), Imm32(0), &notZero);
masm.branchTest32(Assembler::Signed, R1.payloadReg(), R1.payloadReg(), &failure);
masm.bind(&notZero);

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> masm.branchTest32(Assembler::NotEqual, R0.payloadReg(), Imm32(0), &notZero);

This should be branch32 btw.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> This should be branch32 btw.

To elaborate a bit: branch32 will emit a cmp instruction, branchTest32 will emit a test instruction. As a rule of thumb, use branch32 when you want to check if two values are equal/not equal/less than/greater than etc. "test" does a bitwise and of its two operands and is usually used with Assembler::Signed, Assembler::Zero, Assembler::NonZero.

Comment 7

[Sankha Narayan Guria [:sankha]]

Attachment: patch v3

Thanks for the explanation. It really cleared up the the things! :)

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8366733 [details] [diff] [review]
patch v3

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

Great, thanks! r=me with comment below addressed.

::: js/src/jit/x64/BaselineIC-x64.cpp
@@ +88,5 @@
>          // Prevent division by 0.
>          masm.branchTest32(Assembler::Zero, ExtractTemp0, ExtractTemp0, &failure);
>  
>          // Prevent negative 0 and -2147483648 / -1.
> +        masm.branchTest32(Assembler::Equal, eax, Imm32(INT32_MIN), &failure);

This should be branch32, same for x86.

Comment 9

[Sankha Narayan Guria [:sankha]]

Attachment: patch v4

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/340cc0f16046

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/340cc0f16046