Closed
Bug 1160911
Opened 9 years ago
Closed 9 years ago
JIT: precise shift right derived result range for all int32 input ranges.
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: dougc, Assigned: dougc)
Details
Attachments
(1 file, 2 obsolete files)
4.67 KB,
patch
|
dougc
:
review+
|
Details | Diff | Splinter Review |
The compilers range derivation for the shift right operation can be more precise. This bug adds precise range derivation for all int32 input ranges. It happens to be possible to derive a precise range with a very simple and fast algorithm. The algorithm has been cross-contributed to V8, BSD Licensed: https://codereview.chromium.org/1121573004/
Assignee | ||
Comment 1•9 years ago
|
||
Might this patch be simple enough to review as is, or will it need documenting too?
Attachment #8600855 -
Flags: review?(nicolas.b.pierron)
Comment 2•9 years ago
|
||
Comment on attachment 8600855 [details] [diff] [review] Precise shift right derived result range for all int32 input ranges. Review of attachment 8600855 [details] [diff] [review]: ----------------------------------------------------------------- Range Analysis code should have at least 2 reviewers. (asking sunfish for review) This code sounds good to me with the following suggestion applied. ::: js/src/jit/RangeAnalysis.cpp @@ +1312,5 @@ > + > + int32_t lhsLower = lhs->lower(); > + int32_t min = Min(Max(lhsLower, 0) >> shiftUpper, lhsLower >> shiftLower); > + int32_t lhsUpper = lhs->upper(); > + int32_t max = Max(lhsUpper >> shiftLower, Min(lhsUpper, -1) >> shiftUpper); I think this code would be easier to read by expanding the implicit conditions behind the min and max. // The lhs bounds are signed, thus the minimum is either the lower bound shift by the smallest shift // if negative or the lower bound shifted by the biggest shift otherwise. And the opposite for the // maximum. int32_t min = lhsLower < 0 ? lhsLower >> shiftLower : lhsLower >> shiftUpper; int32_t max = lhsUpper > 0 ? rhsUpper >> shiftLower : rhsUpper >> shiftUpper;
Attachment #8600855 -
Flags: review?(sunfish)
Attachment #8600855 -
Flags: review?(nicolas.b.pierron)
Attachment #8600855 -
Flags: review+
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #2) > Comment on attachment 8600855 [details] [diff] [review] ... > I think this code would be easier to read by expanding the implicit > conditions behind the min and max. > > // The lhs bounds are signed, thus the minimum is either the lower bound > shift by the smallest shift > // if negative or the lower bound shifted by the biggest shift otherwise. > And the opposite for the > // maximum. > int32_t min = lhsLower < 0 ? lhsLower >> shiftLower : lhsLower >> > shiftUpper; > int32_t max = lhsUpper > 0 ? rhsUpper >> shiftLower : rhsUpper >> > shiftUpper; Yes the above is equivalent, and I'll change it to use the above. Thank you for the quick review.
Comment 4•9 years ago
|
||
Comment on attachment 8600855 [details] [diff] [review] Precise shift right derived result range for all int32 input ranges. Review of attachment 8600855 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, with nbp's comment :).
Attachment #8600855 -
Flags: review?(sunfish) → review+
Assignee | ||
Comment 5•9 years ago
|
||
Thank you for the review. This patch changes the code as requested in comment 2. Carrying forward r+'s.
Attachment #8600855 -
Attachment is obsolete: true
Attachment #8603794 -
Flags: review+
Assignee | ||
Comment 6•9 years ago
|
||
Requested change in comment 2 was not quite right, but I understood the intent. Carrying forward r+'s.
Attachment #8603794 -
Attachment is obsolete: true
Attachment #8603796 -
Flags: review+
Assignee | ||
Comment 7•9 years ago
|
||
Requesting checkin. Try build has some timeouts but they do not appear related to this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5edef0ffa88c
Keywords: checkin-needed
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a5401e748b13
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•