Closed Bug 1239068 Opened 4 years ago Closed 4 years ago

Inline "PossiblyTypedArrayLength" intrinsic

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: mrrrgn, Assigned: mrrrgn)

References

Details

Attachments

(1 file, 2 obsolete files)

Depends on: 1187232
Assignee: nobody → winter2718
Attached patch inline.diff (obsolete) — Splinter Review
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)
Attached patch inline.diff (obsolete) — Splinter Review
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)
(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
Also, thanks again for the patience, and the reviews.
Attached patch inline.diffSplinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/338294fa149d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.