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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Marty Rosenberg [:mjrosenb]]

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.

Comment 1

[Marty Rosenberg [:mjrosenb]]

Attachment: Add bailouts to the divide codegen.

Comment 2

[Marty Rosenberg [:mjrosenb]]

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 3

[Chris Leary [:cdleary] (not checking bugmail)]

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.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

(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 5

[Jacob Bramley [:jbramley]]

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.