Closed Bug 925586 Opened 7 years ago Closed 7 years ago

cleanup the Ursh special case

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: sunfish, Assigned: sunfish)

References

Details

Attachments

(4 files)

Bug 923659 discusses the distinction between ranges before and after bailout checks. The trajectory of the patches is to remove uses of the extendUInt32ToInt32Min() function, which is used to adjust pre-bailout unsigned ranges, since it is more consistent to adjust post-bailout unsigned ranges instead.

The attached patch finishes this task, removing the last use of extendUInt32ToInt32Min() and pre-bailout adjustment, and establishing the desired post-bailout special-case code for Ursh.
Attachment #815655 - Flags: review?(nicolas.b.pierron)
Moving this patch from bug 923659.
Attachment #815696 - Flags: review?(nicolas.b.pierron)
Moving this patch from bug 923659.
Attachment #815697 - Flags: review?(nicolas.b.pierron)
Attachment #815696 - Flags: review?(nicolas.b.pierron) → review+
Attachment #815697 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 815655 [details] [diff] [review]
range-ursh-special-case.patch

Review of attachment 815655 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/LIR-Common.h
@@ +2027,5 @@
>      { }
>  
>      const char *extraName() const {
> +        if (bitop() == JSOP_URSH && mir_->toUrsh()->bailoutsDisabled())
> +            return "ursh:BailoutsDisabled";

nit: Capitalize the U.
Comment on attachment 815655 [details] [diff] [review]
range-ursh-special-case.patch

Review of attachment 815655 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I forgot to r+ during my previous review.
Attachment #815655 - Flags: review?(nicolas.b.pierron) → review+
This eliminates the use of extendUInt32ToInt32Min in MLoadTypedArrayElement and MLoadTypedArrayElementStatic, making them more precise in the process, and adds comments and an assert describing the reasons why it's safe to do so.
Attachment #816839 - Flags: review?(nicolas.b.pierron)
Attachment #816839 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> Comment on attachment 815655 [details] [diff] [review]
> range-ursh-special-case.patch
> 
> Review of attachment 815655 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/LIR-Common.h
> @@ +2027,5 @@
> >      { }
> >  
> >      const char *extraName() const {
> > +        if (bitop() == JSOP_URSH && mir_->toUrsh()->bailoutsDisabled())
> > +            return "ursh:BailoutsDisabled";
> 
> nit: Capitalize the U.

I changed it to be uncapitalized for consistency with the js_CodeName[jsop_] strings which are used on the else path here, which all begin with lower-case letters. I'm open to changing it back if you think it's best to do so though.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e58a7a4c393
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2a8732eeaef
https://hg.mozilla.org/integration/mozilla-inbound/rev/29a1d64653e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f0ebd310f1d
Depends on: 928625
Depends on: 940864
You need to log in before you can comment on or make changes to this bug.