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)

37 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: dougc, Assigned: dougc)

Details

Attachments

(1 file, 2 obsolete files)

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/
Might this patch be simple enough to review as is, or will it need documenting too?
Attachment #8600855 - Flags: review?(nicolas.b.pierron)
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+
(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 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+
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+
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+
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
https://hg.mozilla.org/mozilla-central/rev/a5401e748b13
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: