Closed Bug 728311 Opened 9 years ago Closed 9 years ago

IonMonkey: IC for JSOP_BINDNAME

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch Patch (obsolete) — Splinter Review
Adds an IC comparable to JM's BINDNAME IC. One difference is that JM has an inline path for the case where the scope chain is the global object (parent is NULL). Since getting the parent requires a few loads nowadays, this case has its own stub now.
Attachment #598925 - Flags: review?(dvander)
Comment on attachment 598925 [details] [diff] [review]
Patch

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

::: js/src/ion/IonCaches.cpp
@@ +132,5 @@
>  
>      Linker linker(masm);
>      IonCode *code = linker.newCode(cx);
> +    if (!code)
> +        return false;

Whoops. Good catch.

@@ +274,5 @@
> +
> +    // If the scope chain is a global object, guard that the parent
> +    // is NULL and move the scope chain into the output reg.
> +
> +    // XXX: do we even need this guard?

You can just guard that scopeChainReg() == scopeChain.

@@ +334,5 @@
> +        // Walk up the scope chain.
> +        while (tobj) {
> +            if (!IsCacheableNonGlobalScope(tobj)) {
> +                IonSpew(IonSpew_InlineCaches, "BINDNAME non-cacheable object");
> +                failures.reset();

|failures.reset()| smells like an anti-pattern. What if we extracted out the IsCacheableNonGlobalScope + |tobj != obj| tests into a helper function? Then we could also avoid testing cacheability twice.

::: js/src/ion/IonCaches.h
@@ +300,5 @@
> +    }
> +
> +    Register scopeChainReg() const { return u.bindname.scopeChain; }
> +    PropertyName *name() const { return u.bindname.name; }
> +    Register outputReg() const { return u.bindname.output; }

super nit: Ion style is for these to be multi-line (this file strays a bit)
Attachment #598925 - Flags: review?(dvander) → review+
Attached patch Patch v2Splinter Review
Addresses review comments + some more changes/refactoring to make the bindname IC look more like the getprop IC. Should hopefully also make it easier to add other *NAME IC's.
Attachment #598925 - Attachment is obsolete: true
Attachment #600418 - Flags: review?(dvander)
Comment on attachment 600418 [details] [diff] [review]
Patch v2

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

::: js/src/ion/CodeGenerator.cpp
@@ +1913,5 @@
> +bool
> +CodeGenerator::visitOutOfLineBindNameCache(OutOfLineCache *ool)
> +{
> +    LBindNameCache *ins = ool->cache()->toBindNameCache();
> +    Register scopeChain = ToRegister(ins->getOperand(0));

Does this deserve a scopeChain() accessor?

@@ +1917,5 @@
> +    Register scopeChain = ToRegister(ins->getOperand(0));
> +    Register output = ToRegister(ins->output());
> +
> +    RegisterSet liveRegs = ins->safepoint()->liveRegs();
> +    liveRegs.maybeTake(output);

Why is this maybeTake necessary?
Attachment #600418 - Flags: review?(dvander) → review+
(In reply to David Anderson [:dvander] from comment #4)
> 
> Does this deserve a scopeChain() accessor?

Done. Some instructions use these accessors and some don't; at some point we should probably decide when/where we want to use them - at least for the more complicated instructions having them may be a bit more readable.

> 
> Why is this maybeTake necessary?

It follows getprop, but now I'm pretty sure the live register set is guaranteed to never contain the output registers anyway (or we'd have seen other failures). So I removed the maybeTake from getprop and bindname.

https://hg.mozilla.org/projects/ionmonkey/rev/0c780caf3eb6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.