Closed Bug 1326437 Opened 3 years ago Closed 3 years ago

Use CacheIR GetName IC in Ion

Categories

(Core :: JavaScript Engine: JIT, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: evilpie, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qf-])

Attachments

(3 files)

The GetName Ion ICs are quite different from the previous baseline ICs that I ported to CacheIR. We should check which of the conditions that Ion checks before attaching a stub are required. We should also port the TypeOfNoProperty stub to CacheIR.
Priority: -- → P2
GetName and BindName are now the only IonCaches ICs left. GetName should be pretty easy to convert and should let us remove a lot of code. I'll probably work on this next week.

After that, BindName will be trivial.
Quite straight-forward after factoring out LookupEnvironment.

I also noticed we don't have spew for GetName yet? I'll file a follow-up bug for that.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8854393 - Flags: review?(evilpies)
This ports an optimization from GenerateEnvironmentChainGuard in IonCaches.cpp: we don't need to shape guard call objects with a non-extensible scope. 

There's a similar optimization for non-configurable properties on the global, but I didn't port that because it means we have to fix up the BaselineInspector too. This code was added a long time ago and nowadays GetName isn't very hot in Ion so I don't think it matters much.
Attachment #8854394 - Flags: review?(evilpies)
I also fixed some minor pre-existing issues.

 11 files changed, 162 insertions(+), 890 deletions(-)
Attachment #8854395 - Flags: review?(evilpies)
Blocks: 1353359
Comment on attachment 8854393 [details] [diff] [review]
Part 1 - Port TypeOfNoProperty to CacheIR

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

As mentioned on IRC this is probably wrong in Ion too. GlobalObjects have a proto-chain that could contains this property, as well as a missing shape guard for it.
Attachment #8854393 - Flags: review?(evilpies)
Comment on attachment 8854394 [details] [diff] [review]
Part 2 - Don't shape guard non-extensible call objects

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

Nice, this seems like the most salvageable part of ion to me.

::: js/src/jit/CacheIR.cpp
@@ +1824,5 @@
> +    // variables). The function might have been relazified under rare
> +    // conditions. In that case, we pessimistically create the guard.
> +    CallObject* callObj = &envObj->as<CallObject>();
> +    JSFunction* fun = &callObj->callee();
> +    if (fun->hasScript() && !fun->nonLazyScript()->funHasExtensibleScope())

Nit: Could be written as !fun->hasScript() || ...HasExtensibleScope()
Attachment #8854394 - Flags: review?(evilpies) → review+
Depends on: 1353384
Comment on attachment 8854395 [details] [diff] [review]
Part 3 - Port GetName IC to CacheIR

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

Looks great!
Attachment #8854395 - Flags: review?(evilpies) → review+
No longer depends on: 1353384
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/406065489474
part 2 - Don't shape guard non-extensible call objects. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/9927b6f58b07
part 3 - Port Ion GetName IC to CacheIR. r=evilpie
I dropped part 1, see bug 1353384 comment 4.
https://hg.mozilla.org/mozilla-central/rev/406065489474
https://hg.mozilla.org/mozilla-central/rev/9927b6f58b07
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1426075
No longer depends on: 1426075
You need to log in before you can comment on or make changes to this bug.