Closed
Bug 1027510
Opened 10 years ago
Closed 10 years ago
Ionmonkey: Add Range analysis for Math.Ceil
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: eternalsushant, Assigned: eternalsushant, Mentored)
Details
Attachments
(1 file, 2 obsolete files)
6.32 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
Make a computeRange function for MCeil.
Assignee | ||
Updated•10 years ago
|
Summary: Ionmonkey: Add Range analysis for Math.ceil → Ionmonkey: Add Range analysis for Math.Ceil
Updated•10 years ago
|
Assignee: nobody → eternalsushant
Mentor: nicolas.b.pierron
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8443519 -
Flags: review?(hv1989)
Comment 2•10 years ago
|
||
Comment on attachment 8443519 [details] [diff] [review] RangeAnalysis for MCeil Review of attachment 8443519 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/RangeAnalysis.cpp @@ +1188,5 @@ > +MCeil::computeRange(TempAllocator &alloc) > +{ > + Range other(input()); > + Range *copy = new(alloc) Range(other); > + copy->resetFractionalPart(); This is not correct, because we need to extend the upper bound in case where we have a fractional part.
Comment 3•10 years ago
|
||
Comment on attachment 8443519 [details] [diff] [review] RangeAnalysis for MCeil Review of attachment 8443519 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/RangeAnalysis.cpp @@ +1188,5 @@ > +MCeil::computeRange(TempAllocator &alloc) > +{ > + Range other(input()); > + Range *copy = new(alloc) Range(other); > + copy->resetFractionalPart(); Has Hannes made me remark, we already include the fractional part in the ranges. 1.5 is represented as [1..2] with fractional part. So reset fractional part is correct for the Int32 range. But the way we compute the exponent does not work the same way as we compute it with as the exponent of the number in Double representation.
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8443519 -
Attachment is obsolete: true
Attachment #8443519 -
Flags: review?(hv1989)
Attachment #8444328 -
Flags: review?(hv1989)
Comment 5•10 years ago
|
||
Comment on attachment 8444328 [details] [diff] [review] RangeAnalysis for MCeil Review of attachment 8444328 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Can you also add a patch that removes "Decrement lower bound" of floor? Since we discussed it isn't needed for ceil. It also isn't for floor. Note: since RA is so hard, we decided that for RA changes patches need 2 reviews. Flagging nbp. ::: js/src/jit/RangeAnalysis.cpp @@ +979,5 @@ > return copy; > } > > +Range * > +Range::mceil(TempAllocator &alloc, const Range *op) s/mceil/ceil/ @@ +986,5 @@ > + > + if (copy->hasInt32Bounds()) > + copy->max_exponent_ = copy->exponentImpliedByInt32Bounds(); > + else if (copy->max_exponent_ < MaxFiniteExponent) > + copy->max_exponent_++; Can you add a comment why we need to increase the exponent?
Attachment #8444328 -
Flags: review?(nicolas.b.pierron)
Attachment #8444328 -
Flags: review?(hv1989)
Attachment #8444328 -
Flags: review+
Comment 6•10 years ago
|
||
Comment on attachment 8444328 [details] [diff] [review] RangeAnalysis for MCeil Review of attachment 8444328 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/RangeAnalysis.cpp @@ +986,5 @@ > + > + if (copy->hasInt32Bounds()) > + copy->max_exponent_ = copy->exponentImpliedByInt32Bounds(); > + else if (copy->max_exponent_ < MaxFiniteExponent) > + copy->max_exponent_++; In the comment that Hannes mentionned, also mention why we can restrict to MaxTruncatableExponent and not increment the value if it is above 2 ** MaxTruncatableExponent. @@ +1225,5 @@ > void > +MCeil::computeRange(TempAllocator &alloc) > +{ > + Range other(getOperand(0)); > + setRange(Range::mceil(alloc,&other)); style-nit: add a space after the comma.
Attachment #8444328 -
Flags: review?(nicolas.b.pierron) → review+
Comment 7•10 years ago
|
||
I'm waiting for an updated patch, before I can get this in the tree.
Flags: needinfo?(eternalsushant)
Assignee | ||
Comment 8•10 years ago
|
||
Sorry for the delay.
Attachment #8444328 -
Attachment is obsolete: true
Attachment #8447267 -
Flags: review?(hv1989)
Flags: needinfo?(eternalsushant)
Updated•10 years ago
|
Attachment #8447267 -
Flags: review?(hv1989) → review+
https://hg.mozilla.org/mozilla-central/rev/f17234995f03
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•