Closed
Bug 708486
Opened 13 years ago
Closed 12 years ago
IonMonkey: Add guards into the code generator for IDIV on ARM
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mjrosenb, Unassigned)
Details
Attachments
(1 file)
3.43 KB,
patch
|
cdleary
:
review+
jbramley
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Attachment #585622 -
Flags: review?(christopher.leary)
Reporter | ||
Comment 2•12 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 3•12 years ago
|
||
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 5•12 years ago
|
||
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+
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•