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

RESOLVED FIXED in mozilla29

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jandem, Assigned: sankha)

Tracking

(Blocks: 1 bug)

unspecified
mozilla29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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..
(Assignee)

Comment 1

5 years ago
Created attachment 8359864 [details] [diff] [review]
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
Attachment #8359864 - Flags: review?(jdemooij)
(Reporter)

Comment 2

5 years ago
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 :)
Attachment #8359864 - Flags: review?(jdemooij)
(Reporter)

Updated

5 years ago
Component: JavaScript Engine → JavaScript Engine: JIT
(Assignee)

Comment 3

5 years ago
Created attachment 8366606 [details] [diff] [review]
patch v2
Attachment #8359864 - Attachment is obsolete: true
Attachment #8366606 - Flags: review?(jdemooij)
(Reporter)

Comment 4

5 years ago
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);
Attachment #8366606 - Flags: review?(jdemooij)
(Reporter)

Comment 5

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

This should be branch32 btw.
(Reporter)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
Created attachment 8366733 [details] [diff] [review]
patch v3

Thanks for the explanation. It really cleared up the the things! :)
Attachment #8366606 - Attachment is obsolete: true
Attachment #8366733 - Flags: review?(jdemooij)
(Reporter)

Comment 8

5 years ago
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.
Attachment #8366733 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 9

5 years ago
Created attachment 8367193 [details] [diff] [review]
patch v4
Assignee: general → sankha93
Attachment #8366733 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/340cc0f16046
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.