Last Comment Bug 762414 - IonMonkey: Implement GET/SETALIASEDVAR
: IonMonkey: Implement GET/SETALIASEDVAR
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: David Anderson [:dvander]
:
:
Mentors:
Depends on: 761685
Blocks: IonSpeed
  Show dependency treegraph
 
Reported: 2012-06-07 02:27 PDT by David Anderson [:dvander]
Modified: 2012-06-14 11:55 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (11.65 KB, patch)
2012-06-07 02:27 PDT, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
patch (11.65 KB, patch)
2012-06-07 02:52 PDT, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
actual patch (13.39 KB, patch)
2012-06-07 02:54 PDT, David Anderson [:dvander]
jdemooij: review+
luke: review+
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2012-06-07 02:27:17 PDT
Created attachment 630899 [details] [diff] [review]
patch

This patch implements the new ALIASEDVAR opcodes in IonMonkey. Luckily, these can be entirely implemented in MIR.
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2012-06-07 02:30:32 PDT
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] 2012-06-07 02:52:36 PDT
Created attachment 630904 [details] [diff] [review]
patch

Slightly better patch, uses TI.
Comment 3 David Anderson [:dvander] 2012-06-07 02:54:29 PDT
Created attachment 630905 [details] [diff] [review]
actual patch

Argh, sorry, forgot to qref.
Comment 4 Jan de Mooij [:jandem] 2012-06-07 06:19:24 PDT
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] 2012-06-07 23:59:40 PDT
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] 2012-06-14 11:55:07 PDT
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

Note You need to log in before you can comment on or make changes to this bug.