Last Comment Bug 684402 - IonMonkey: Compile JSOP_GETGNAME
: IonMonkey: Compile JSOP_GETGNAME
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: David Anderson [:dvander]
:
Mentors:
: 678387 (view as bug list)
Depends on: 683407 689325
Blocks: 684381 685228 692114 693432
  Show dependency treegraph
 
Reported: 2011-09-02 16:32 PDT by David Anderson [:dvander]
Modified: 2011-10-13 02:38 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP v0 (25.79 KB, patch)
2011-09-06 19:48 PDT, David Anderson [:dvander]
no flags Details | Diff | Review
wip v1 (66.71 KB, patch)
2011-10-09 20:35 PDT, David Anderson [:dvander]
no flags Details | Diff | Review
wip v2 (71.54 KB, patch)
2011-10-10 00:24 PDT, David Anderson [:dvander]
no flags Details | Diff | Review
part 1: everything but ARM (80.59 KB, patch)
2011-10-10 11:37 PDT, David Anderson [:dvander]
bhackett1024: review+
sstangl: review+
Details | Diff | Review

Description David Anderson [:dvander] 2011-09-02 16:32:45 PDT
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] 2011-09-06 19:48:53 PDT
Created attachment 558711 [details] [diff] [review]
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 2 Sean Stangl [:sstangl] 2011-09-21 12:53:10 PDT
*** Bug 678387 has been marked as a duplicate of this bug. ***
Comment 3 David Anderson [:dvander] 2011-10-09 20:35:10 PDT
Created attachment 565854 [details] [diff] [review]
wip v1

Adds TypeBarrier, GuardShape, LoadSlots, and LoadSlotV instructions. Remaining work to do:
 * x64, ARM support
 * typed loads
 * tests
Comment 4 David Anderson [:dvander] 2011-10-10 00:24:41 PDT
Created attachment 565872 [details] [diff] [review]
wip v2

Adds x64 support, typed loads. ARM and actual barrier functionality soon.
Comment 5 David Anderson [:dvander] 2011-10-10 11:37:20 PDT
Created attachment 565983 [details] [diff] [review]
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] 2011-10-12 12:35:09 PDT
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] 2011-10-12 14:42:29 PDT
> 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] 2011-10-13 02:38:59 PDT
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.

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