IonMonkey: Compile JSOP_NAME

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: nbp, Assigned: bhackett)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Necessary for benchmarks.
(Assignee)

Updated

6 years ago
Assignee: general → bhackett1024
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
Created attachment 584299 [details] [diff] [review]
patch (edde637d2661)
Attachment #583047 - Attachment is obsolete: true
Attachment #584299 - Flags: review?(dvander)
(Assignee)

Comment 3

6 years ago
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).
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+
(Assignee)

Comment 5

6 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/b26b0782dd94
Depends on: 717494

Comment 6

6 years ago
Can this be closed?
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.