684402 - IonMonkey: Compile JSOP_GETGNAME

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

The easiest of the property access opcodes - this should start showing us how to integrate Type Inference information for heap accesses as well.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: WIP v0

Provides MSlots, MLoadSlot, LSlots, LLoadSlotV. With this we can run scripts with GETGLOBAL. Left todo:
 * Integrate with TI and add typed LIR.
 * x64 support.
 * Tests.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: wip v1

Adds TypeBarrier, GuardShape, LoadSlots, and LoadSlotV instructions. Remaining work to do:
 * x64, ARM support
 * typed loads
 * tests

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: wip v2

Adds x64 support, typed loads. ARM and actual barrier functionality soon.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1: everything but ARM

Brian, would you mind looking at the TI integration in IonBuilder.cpp and TypeOracle.cpp?

Comment 6

[Sean Stangl [:sstangl]]

Comment on attachment 565983 [details] [diff] [review]
part 1: everything but ARM

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

::: js/src/ion/Bailouts.cpp
@@ +246,5 @@
>          JS_NOT_REACHED("NYI");
>      }
>  
> +    // TypeBarriers on non-idempotent ops not supported yet.
> +    JS_ASSERT(iter.bailoutKind() == Bailout_Normal);

For the sake of fuzzing, it's useful for NYI assertions to contain the string "NYI":
if (iter.bailoutKind() != Bailout_Normal)
    JS_NOT_REACHED("NYI: TypeBarriers on non-idempotent ops.");

::: js/src/ion/IonBuilder.cpp
@@ +1907,5 @@
> +    // prototype chain, then if obj has a defined property now, and the
> +    // property has a default or method shape, the only way it can produce
> +    // undefined in the future is if it is deleted. Deletion causes type
> +    // properties to be explicitly marked with undefined.
> +    *result = false;

Suggest "isKnownConstant" instead of "result".

@@ +1949,5 @@
> +    // instruction is not idempotent, even if it has a singleton type,
> +    // there must be a resume point capturing the original def.
> +    current->push(ins);
> +    if (!ins->isIdempotent() && !resumeAfter(ins))
> +        return false;

jsop_call() also creates a resume point after the operation. Presumably it wants to do so whether or not a type barrier needs to follow. That is fine, but the logic is sort of hidden by the naming.

This function's intent is to push a type barrier if required; otherwise, to push |ins|.

"pushInstructionWithTypeBarrier()"?

@@ +1952,5 @@
> +    if (!ins->isIdempotent() && !resumeAfter(ins))
> +        return false;
> +
> +    if (!observed || observed->unknown())
> +        return true;

Renaming to something like the above would also make more obvious errors where |ins| is duplicated on the stack by the caller of this function.

::: js/src/ion/IonMacroAssembler.h
@@ +145,5 @@
>  
>      // Emits a test of a value against all types in a TypeSet. A scratch
>      // register is required.
> +    template <typename T>
> +    void testTypeSet(const T &address, types::TypeSet *types, Register scratch0,

nit: s/0//

::: js/src/ion/LIR-Common.h
@@ +715,5 @@
> +    }
> +};
> +
> +// Load a value from an object's dslots or a slots vector.
> +class LLoadSlotT : public LInstructionHelper<1, 1, 0>

Comment should distinguish from LLoadSlotV by mentioning the typed load.

::: js/src/ion/MIR.h
@@ +1011,5 @@
> +  public:
> +    enum Mode {
> +        Fallible,
> +        Infallible,
> +        TypeBarrier

Comment on effect of each mode?

@@ +1040,5 @@
> +    MDefinition *input() const {
> +        return getOperand(0);
> +    }
> +    BailoutKind bailoutKind() const {
> +        JS_ASSERT(fallible());

This assertion looks strange, but its utility makes sense. Maybe a comment would help.

::: js/src/ion/Snapshots.cpp
@@ +51,5 @@
>  // Snapshot structure:
>  //
>  //   [ptr] Debug only: JSScript *
>  //   [vwu] pc offset
> +//   [vwu] bits (n-31]: # of slots, including nargs (max 65535).

Well... max 2^(32-n). We should have this value available as a constant, so we can abort on overfullness during SSA construction.

::: js/src/ion/TypeOracle.h
@@ +85,5 @@
>      virtual types::TypeSet *parameterTypeSet(JSScript *script, size_t index) { return NULL; }
> +    virtual types::TypeSet *propertyRead(JSScript *script, jsbytecode *pc,
> +                                         types::TypeSet **barrier) {
> +        *barrier = NULL;
> +        return NULL;

Can be "return (*barrier = NULL);", if that helps condense to the above line.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

> For the sake of fuzzing, it's useful for NYI assertions to contain the
> string "NYI":
> if (iter.bailoutKind() != Bailout_Normal)
>     JS_NOT_REACHED("NYI: TypeBarriers on non-idempotent ops.");

Okay, I'll keep that in mind for the future - this code goes away in a follow-up patch.

> 
> Suggest "isKnownConstant" instead of "result".

Good idea.

> This function's intent is to push a type barrier if required; otherwise, to
> push |ins|.
> 
> "pushInstructionWithTypeBarrier()"?

This stuff is all wrong anyway, it's fixed in the "type barriers for calls" patch. You're right though, the resume point should be created outside of the type barrier logic.

> Well... max 2^(32-n). We should have this value available as a constant, so
> we can abort on overfullness during SSA construction.

nargs is limited to 65535 by the emitter (same for nslots and therefore nfixed).

Pushed with nits fixed: http://hg.mozilla.org/projects/ionmonkey/rev/e14a523e99d3

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 565983 [details] [diff] [review]
part 1: everything but ARM

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

::: js/src/ion/IonLIR.h
@@ +988,5 @@
> +# define BOX_OUTPUT_ACCESSORS()                                             \
> +    const LDefinition *outputValue() {                                      \
> +        return getDef(0);                                                   \
> +    }
> +#endif

I think we may want some kind of LValueDefinition to factor these "if defined(JS_NUNBOX32)" & "elif defined(JS_PUNBOX64)".  Your current implementation implies that any code that has to use a Value must either reproduce this "#ifdef" or do 2 implementation of the same function based on the architecture.  These separation caused by Value representation appear in the Lowering-shared-inl.h and in specialized Lowering implementation.