Last Comment Bug 701966 - IonMonkey: Compile JSOP_NAME
: IonMonkey: Compile JSOP_NAME
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Brian Hackett (:bhackett)
:
Mentors:
Depends on: 717494
Blocks: 684381
  Show dependency treegraph
 
Reported: 2011-11-11 18:01 PST by Nicolas B. Pierron [:nbp]
Modified: 2012-01-30 13:28 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (bf524b56351f) (47.47 KB, patch)
2011-12-19 19:14 PST, Brian Hackett (:bhackett)
no flags Details | Diff | Review
patch (edde637d2661) (42.75 KB, patch)
2011-12-25 18:47 PST, Brian Hackett (:bhackett)
no flags Details | Diff | Review
patch (4b6b2d6421f9) (42.78 KB, patch)
2012-01-03 08:25 PST, Brian Hackett (:bhackett)
dvander: review+
Details | Diff | Review

Description Nicolas B. Pierron [:nbp] 2011-11-11 18:01:53 PST
Necessary for benchmarks.
Comment 1 Brian Hackett (:bhackett) 2011-12-19 19:14:37 PST
Created attachment 583047 [details] [diff] [review]
WIP (bf524b56351f)

Barely working patch to add a CompileInfo slot to keep track of the current scope chain, compile NAME and CALLNAME.
Comment 2 Brian Hackett (:bhackett) 2011-12-25 18:47:17 PST
Created attachment 584299 [details] [diff] [review]
patch (edde637d2661)
Comment 3 Brian Hackett (:bhackett) 2012-01-03 08:25:14 PST
Created attachment 585417 [details] [diff] [review]
patch (4b6b2d6421f9)

Rebased, with tweak to avoid loading the scope chain in frames which will not use it (would be redundant with dead code elimination).
Comment 4 David Anderson [:dvander] 2012-01-05 18:52:22 PST
Comment on attachment 585417 [details] [diff] [review]
patch (4b6b2d6421f9)

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

r=me with some small refactorings to MIR/LIR

::: js/src/ion/CodeGenerator.cpp
@@ +562,2 @@
>      Label mismatched;
> +    for (uint32 i = 1; i < CountArgSlots(gen->info().fun()); i++) {

It'd be nice if this 1 here and in (i - 1) below were a constant somewhere, akin to THIS_SLOT. like FIRST_ARGUMENT or something

::: js/src/ion/IonBuilder.cpp
@@ +382,5 @@
> +
> +    // The scope chain is only tracked in scripts that have NAME opcodes which
> +    // will try to access the scope. For other scripts, the scope instructions
> +    // will be dead but without a dead code elimination pass their code will
> +    // still be generated.

We currently have such a pass, but the slot will be held live by resume pointers. That's okay though, it won't generate any LIR and snapshots have special encoding for constants.

::: js/src/ion/LIR-Common.h
@@ +776,5 @@
>      }
>  };
>  
> +// Materialize a JSObject stored in an interpreter frame for OSR.
> +class LOsrObject : public LInstructionHelper<1, 1, 0>

Similar comment here, whether this can be LOsrScopeChain or something.

@@ +1161,5 @@
> +        return getDef(0);
> +    }
> +};
> +
> +class LCallGetPropertyOrName : public LVMCallInstructionHelper<LDefinition::BOX, BOX_PIECES, 1, 0>

This should be split up as the MIR.

::: js/src/ion/MIR.h
@@ +668,5 @@
> +    }
> +
> +  public:
> +    INSTRUCTION_HEADER(Callee);
> +    static MCallee *New() { return new MCallee(); }

nit: local style is
    static MCallee *New() {
        return new MCallee();
    }

@@ +1812,5 @@
>  };
>  
> +// MIR representation of a JSObject pointer on the OSR StackFrame.
> +// The pointer is indexed off of OsrFrameReg.
> +class MOsrObject : public MAryInstruction<1>

Do we plan on this being used for things other than the scope chain? If not, can we call this something like MOsrScopeChain (and then get rid of the frameOffset field)?

@@ +2392,5 @@
> +    enum AccessKind {
> +        PROPERTY,
> +        NAMETYPEOF,
> +        NAME
> +    };

Here we should have two separate opcodes, MGetPropertyGeneric (or just MGetProperty) and MGetName or something. You could share everything in a base class, like MPropertyNameInstruction or whatever, but we should have at least two distinct ops.
Comment 5 Brian Hackett (:bhackett) 2012-01-09 16:45:51 PST
https://hg.mozilla.org/projects/ionmonkey/rev/b26b0782dd94
Comment 6 Paul Wright 2012-01-24 10:25:58 PST
Can this be closed?

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