762414 - IonMonkey: Implement GET/SETALIASEDVAR

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: patch

This patch implements the new ALIASEDVAR opcodes in IonMonkey. Luckily, these can be entirely implemented in MIR.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 630899 [details] [diff] [review]
patch

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

::: js/src/ion/IonBuilder.h
@@ +359,5 @@
>      bool jsop_iternext(uint8 depth);
>      bool jsop_itermore();
>      bool jsop_iterend();
> +    bool jsop_getaliasedvar(ScopeCoordinate sc);
> +    bool jsop_setaliasedvar(ScopeCoordinate sc);

Why do these return bool?

Comment 2

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

Attachment: patch

Slightly better patch, uses TI.

Comment 3

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

Attachment: actual patch

Argh, sorry, forgot to qref.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 630905 [details] [diff] [review]
actual patch

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

::: js/src/ion/IonBuilder.cpp
@@ +924,5 @@
> +        return true;
> +
> +      case JSOP_SETALIASEDVAR:
> +        jsop_setaliasedvar(ScopeCoordinate(pc));
> +        return true;

"return jsop_getaliasedvar(..)" and "return jsop_setaliasedvar(..)"

@@ +4784,5 @@
> +                            : MConstant::New(UndefinedValue());
> +        current->add(ins);
> +        current->push(ins);
> +        return true;
> +    }

Nit: you can use pushConstant(NullValue()) etc here.

::: js/src/ion/MIR.h
@@ +4433,5 @@
> +    MEnclosingScope(MDefinition *obj)
> +      : MLoadFixedSlot(obj, ScopeObject::enclosingScopeSlot())
> +    {
> +        setResultType(MIRType_Object);
> +        setMovable();

I think LoadFixedSlot should have setMovable() like LoadSlot. Can you move this call to LoadFixedSlot?

::: js/src/ion/TypeOracle.cpp
@@ +542,5 @@
> +    if (varTypes) {
> +        varTypes->addFreeze(cx);
> +        return getMIRType(varTypes);
> +    }
> +    return MIRType_Value;

pushedTypes(0) may be more precise. You also don't need the addFreeze, getMIRType calls getKnownTypeTag and this adds a constraint to trigger recompilation if a new type is added.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 630905 [details] [diff] [review]
actual patch

Everyone looks so nice...

On a random note: we've talked about how IM can common/hoist scope loads, but what about the variables themselves?  How much work would it be for IM to track ranges where an aliased var cannot be read/written (viz., by calls out to nested functions) and thus keep consecutive reads/writes to aliased vars entirely in SSA?  For example, if you had:

  function f(x,y) {
     x += 5;
     y += x;
     (function() { print(x + y) })()
  }

keeping x and y in registers for the first two statements and then flushing their values (to the scope objects) right before the lambda call.

I guess doing this isn't much easier than doing the same thing for object property reads/writes except that you know more about the object.

Comment 6

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

If I understand the question right, I think GVN + AA could maybe handle that if we inlined the function (and would need extra instrumentation if we didn't).

http://hg.mozilla.org/projects/ionmonkey/rev/7ab88528503e