IonMonkey: Implement GET/SETALIASEDVAR

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 630899 [details] [diff] [review]
patch

This patch implements the new ALIASEDVAR opcodes in IonMonkey. Luckily, these can be entirely implemented in MIR.
Attachment #630899 - Flags: review?(jdemooij)
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?
(Assignee)

Comment 2

5 years ago
Created attachment 630904 [details] [diff] [review]
patch

Slightly better patch, uses TI.
Attachment #630899 - Attachment is obsolete: true
Attachment #630899 - Flags: review?(jdemooij)
Attachment #630904 - Flags: review?(jdemooij)
(Assignee)

Comment 3

5 years ago
Created attachment 630905 [details] [diff] [review]
actual patch

Argh, sorry, forgot to qref.
Attachment #630904 - Attachment is obsolete: true
Attachment #630904 - Flags: review?(jdemooij)
Attachment #630905 - Flags: review?(jdemooij)
(Assignee)

Updated

5 years ago
Attachment #630905 - Flags: review?(luke)
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.
Attachment #630905 - Flags: review?(jdemooij) → review+

Comment 5

5 years ago
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.
Attachment #630905 - Flags: review?(luke) → review+
(Assignee)

Comment 6

5 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.