Inline "PossiblyTypedArrayLength" intrinsic

RESOLVED FIXED in Firefox 46

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mrrrgn, Assigned: mrrrgn)

Tracking

unspecified
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Updated

2 years ago
Depends on: 1187232
(Assignee)

Updated

2 years ago
Assignee: nobody → winter2718
(Assignee)

Comment 1

2 years ago
Created attachment 8708023 [details] [diff] [review]
inline.diff
Attachment #8708023 - Flags: review?(jwalden+bmo)
Comment on attachment 8708023 [details] [diff] [review]
inline.diff

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

::: js/src/jit/MCallOptimize.cpp
@@ +2269,5 @@
>  IonBuilder::InliningStatus
> +IonBuilder::inlinePossiblyWrappedTypedArrayLength(CallInfo& callInfo)
> +{
> +    // inlineTypedArrayLength will not inline wrapped objects since they do not
> +    // pass an IsTypedArrayObject test, which is what we want.

Huh, no kidding.  Mini-nit, tho: inside inlineTypedArrayLength is a stale comment about how the intrinsic is unreachable if the argument isn't a typed array, which (depending how you read it) is no longer true after this patch.  Could you flip these, so that inlineTypedArrayLength calls the Possibly variant, and move the stale comment (adjusted as necessary) into that method?  Just want to be clear that Possibly *does* have to consider that its argument might not be a typed array, where non-Possibly is just being slightly pessimal (and dealing with the possibility it's generating unreachable code).

Given I'm not sure what the comment wordings would look like for this, let's do one mini-pass on reviewing the updated patch.  This otherwise looks fine.
Attachment #8708023 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

2 years ago
Created attachment 8708448 [details] [diff] [review]
inline.diff

Holy cow, good catch. I removed the last sentence, that suggested the code should never be reached.
Attachment #8708023 - Attachment is obsolete: true
Attachment #8708448 - Flags: review?(jwalden+bmo)
Comment on attachment 8708448 [details] [diff] [review]
inline.diff

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

::: js/src/jit/MCallOptimize.cpp
@@ +2269,5 @@
> +IonBuilder::inlinePossiblyWrappedTypedArrayLength(CallInfo& callInfo)
> +{
> +    // inlineTypedArrayLength will not inline wrapped objects since they do not
> +    // pass an IsTypedArrayObject test, which is what we want.
> +    return inlineTypedArrayLength(callInfo);

I meant to have these flipped -- name the *existing* method inlinePossiblyWrappedTypedArrayLength, then have inlineTypedArrayLength call iPWTAL.
Attachment #8708448 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 5

2 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> Comment on attachment 8708448 [details] [diff] [review]
> inline.diff
> 
> Review of attachment 8708448 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/MCallOptimize.cpp
> @@ +2269,5 @@
> > +IonBuilder::inlinePossiblyWrappedTypedArrayLength(CallInfo& callInfo)
> > +{
> > +    // inlineTypedArrayLength will not inline wrapped objects since they do not
> > +    // pass an IsTypedArrayObject test, which is what we want.
> > +    return inlineTypedArrayLength(callInfo);
> 
> I meant to have these flipped -- name the *existing* method
> inlinePossiblyWrappedTypedArrayLength, then have inlineTypedArrayLength call
> iPWTAL.

Me IRL: http://img13.deviantart.net/20eb/i/2012/153/e/a/whoosh_by_medli20-d520mia.gif
(Assignee)

Comment 6

2 years ago
Also, thanks again for the patience, and the reviews.
(Assignee)

Comment 7

2 years ago
Created attachment 8709630 [details] [diff] [review]
inline.diff
Attachment #8708448 - Attachment is obsolete: true
Attachment #8709630 - Flags: review?(jwalden+bmo)
Comment on attachment 8709630 [details] [diff] [review]
inline.diff

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

::: js/src/jit/MCallOptimize.cpp
@@ +2253,5 @@
>          return InliningStatus_NotInlined;
>      if (getInlineReturnType() != MIRType_Int32)
>          return InliningStatus_NotInlined;
>  
>      // Note that the argument we see here is not necessarily a typed array.

This comment could probably go away now that it's obvious the method can be given non-typed arrays.
Attachment #8709630 - Flags: review?(jwalden+bmo) → review+

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/338294fa149d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.