IonMonkey: Add guards into the code generator for IDIV on ARM

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mjrosenb, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
In order to compile idiv, we currently generate a call to libc's implementation of divide.
This is problematic because it returns the wrong value when we divide by 0, and INT_MIN / -1.
Guards should be added in.  Also I believe some amount of optimization can be added to the code.
(Reporter)

Comment 1

6 years ago
Created attachment 585622 [details] [diff] [review]
Add bailouts to the divide codegen.
Attachment #585622 - Flags: review?(christopher.leary)
(Reporter)

Comment 2

6 years ago
Jacob, This is the type of patch that you normally review, but I haven't seen you for a while.  Feel free to review this if you are around.
Comment on attachment 585622 [details] [diff] [review]
Add bailouts to the divide codegen.

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

Nice use of predicates in this patch.

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +483,3 @@
>      Register lhs = ToRegister(ins->lhs());
>      Register rhs = ToRegister(ins->rhs());
> +#if 0 // informative, but not actually needed.  Handled by other checks.

Nit: Don't some compilers complain when you put C comments in a preprocessor line? I'd remove the if 0 section in lieu of a comment or hoist the comment above the if 0.

@@ +495,2 @@
>          return false;
> +    // Prevent 0 / X (with X < 0) and X / 0

Could we prefix this with a comment explaining that there are values that we can't place into an integer result (i.e. -0 and NaN). I think that makes the subsequent breakdown more clear.

@@ +509,5 @@
> +    masm.setupAlignedABICall(2);
> +    masm.setABIArg(0, lhs);
> +    masm.setABIArg(1, rhs);
> +    masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, __aeabi_idiv));
> +    masm.ma_cmp(r3, Imm32(0));

We should definitely have a comment that r3 is where the modulus happens to be stored, maybe with a fixme bug to replicate the intrinsic into an Ion assembly snippet.
Attachment #585622 - Flags: review?(christopher.leary) → review+
(In reply to Chris Leary [:cdleary] from comment #3)
>       I'd remove the if 0 section in lieu of a comment

^ Yes, please! We should try not to add any more #if 0s (and to remove existing ones)
Comment on attachment 585622 [details] [diff] [review]
Add bailouts to the divide codegen.

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

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +509,5 @@
> +    masm.setupAlignedABICall(2);
> +    masm.setABIArg(0, lhs);
> +    masm.setABIArg(1, rhs);
> +    masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, __aeabi_idiv));
> +    masm.ma_cmp(r3, Imm32(0));

Agreed. I couldn't work out why you were reading r3 until I read Chris's comment.
Attachment #585622 - Flags: review+
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.