Closed Bug 701966 Opened 13 years ago Closed 12 years ago

IonMonkey: Compile JSOP_NAME

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: nbp, Assigned: bhackett1024)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Necessary for benchmarks.
Assignee: general → bhackett1024
Attached patch WIP (bf524b56351f) (obsolete) — Splinter Review
Barely working patch to add a CompileInfo slot to keep track of the current scope chain, compile NAME and CALLNAME.
Attached patch patch (edde637d2661) (obsolete) — Splinter Review
Attachment #583047 - Attachment is obsolete: true
Attachment #584299 - Flags: review?(dvander)
Rebased, with tweak to avoid loading the scope chain in frames which will not use it (would be redundant with dead code elimination).
Attachment #584299 - Attachment is obsolete: true
Attachment #584299 - Flags: review?(dvander)
Attachment #585417 - Flags: review?(dvander)
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.
Attachment #585417 - Flags: review?(dvander) → review+
Can this be closed?
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.