728311 - IonMonkey: IC for JSOP_BINDNAME

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

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.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

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)

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch v2

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.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

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?

Comment 5

[Jan de Mooij [:jandem]]

(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