Last Comment Bug 708486 - IonMonkey: Add guards into the code generator for IDIV on ARM
: IonMonkey: Add guards into the code generator for IDIV on ARM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Linux
: -- normal (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-07 17:27 PST by Marty Rosenberg [:mjrosenb]
Modified: 2012-01-30 13:29 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add bailouts to the divide codegen. (3.43 KB, patch)
2012-01-03 18:24 PST, Marty Rosenberg [:mjrosenb]
cdleary: review+
Jacob.Bramley: review+
Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2011-12-07 17:27:46 PST
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] 2012-01-03 18:24:26 PST
Created attachment 585622 [details] [diff] [review]
Add bailouts to the divide codegen.
Comment 2 Marty Rosenberg [:mjrosenb] 2012-01-03 18:26:13 PST
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) 2012-01-04 13:54:24 PST
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] 2012-01-04 14:00:18 PST
(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] 2012-01-05 03:30:10 PST
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.

Note You need to log in before you can comment on or make changes to this bug.