Last Comment Bug 700303 - IonMonkey: Compile JSOP_SETGNAME
: IonMonkey: Compile JSOP_SETGNAME
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 701954 (view as bug list)
Depends on:
Blocks: 684381 701993
  Show dependency treegraph
 
Reported: 2011-11-07 07:26 PST by Jan de Mooij [:jandem]
Modified: 2011-12-01 12:11 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (23.80 KB, patch)
2011-11-07 13:25 PST, Jan de Mooij [:jandem]
dvander: review+
bhackett1024: review+
Details | Diff | Splinter Review
Patch v2 (29.16 KB, patch)
2011-11-08 05:08 PST, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2011-11-07 07:26:17 PST
Patch is almost done but I want to test this a bit more before asking for review.
Comment 1 Jan de Mooij [:jandem] 2011-11-07 13:25:03 PST
Created attachment 572594 [details] [diff] [review]
Patch v1

Not sure about some things like the globalPropertyWrite function, suggestions would be very welcome.
Comment 2 Jan de Mooij [:jandem] 2011-11-07 13:34:01 PST
Btw, this patch disables branding. It caused some jit-test failures and branding will be removed as part of Brian's ObjShrink work.

Will IM require compile-and-go? The implementation of bindgname, for instance, will be a bit more complex without compile-and-go...
Comment 3 David Anderson [:dvander] 2011-11-07 14:54:11 PST
Comment on attachment 572594 [details] [diff] [review]
Patch v1

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

Looks good, mostly small comments. One concern is that if you have, like:
   a.x = 50

We'll pick StoreSlotT (I think?) and end up allocating a register to store the immediate, which isn't always ideal. We should support embedding non-double constants in StoreSlotT, at least on x86/x64.

::: js/src/ion/IonBuilder.cpp
@@ +2186,5 @@
> +    if (!shape->hasSlot())
> +        return abort("SETGNAME property has no slot");
> +
> +    if (propertyTypes && propertyTypes->isOwnProperty(cx, globalObj->getType(cx), true))
> +        return abort("SETGNAME property is non-configurable, non-enumerable or non-writable");

"isOwnProperty" means "this property is directly on the object, not on the prototype".

@@ +2203,5 @@
> +    current->add(slots);
> +
> +    MDefinition *value = current->pop();
> +    MStoreSlot *store = MStoreSlot::New(slots, shape->slot - globalObj->numFixedSlots(), value);
> +    current->add(store);

There must be an:

    if (!resumeAfter(store))
        return false;

after here, and I think, before the current->pop(). Since a write to the heap is effectful, it's not safe to repeat it, so we have to capture a new resume point.

::: js/src/ion/LIR-Common.h
@@ +743,5 @@
> +class LStoreSlotV : public LInstructionHelper<0, 1 + BOX_PIECES, 0>
> +{
> +  public:
> +    LIR_HEADER(StoreSlotV);
> +    BOX_OUTPUT_ACCESSORS();

This instruction has no outputs, right?

@@ +761,5 @@
> +};
> +
> +// Store a typed value to an object's dslots or a slots vector. Unlike
> +// LStoreSlotV, this can bypass storing a type tag.
> +class LStoreSlotT : public LInstructionHelper<0, 2, 0>

It might be worth explaining the "can bypass" here. We need to know that the slot being written to is known to only hold one type. Otherwise, what StoreSlotT gets us is better register allocation (we can embed constants or FP regs instead of using a second register).

::: js/src/ion/MIR.h
@@ +1663,5 @@
> +        return this;
> +    }
> +    MDefinition *to() const {
> +        return getOperand(0);
> +    }

Maybe "slots" instead of "to"? (Not sure what MLoadSlot has, but if it has "from", slots sounds better there too.)

::: js/src/ion/TypeOracle.h
@@ +88,5 @@
>          *barrier = NULL;
>          return NULL;
>      }
> +    virtual types::TypeSet *globalPropertyWrite(JSScript *script, jsbytecode *pc,
> +                                                jsid id, bool *monitored) {

nit: Can we call this "canSpecialize" instead of "monitored"? This makes it more amenable to non-TI oracles.

::: js/src/ion/x64/MacroAssembler-x64.h
@@ +116,5 @@
>      void moveValue(const Value &val, const Register &dest) {
>          movq(ImmWord((void *)val.asRawBits()), dest);
>      }
> +    void boxValue(JSValueShiftedTag tag, const Operand &src, const Register &dest) {
> +        movq(ImmWord(tag), dest);

Nit: ImmShiftedTag

::: js/src/ion/x86/CodeGenerator-x86.cpp
@@ +310,5 @@
> +    }
> +
> +    // Store the type tag if needed.
> +    if (valueType != store->mir()->slotType())
> +        masm.movl(Imm32(MIRTypeToTag(valueType)), Operand(base, offset + NUNBOX32_TYPE_OFFSET));

nit: ImmTag

::: js/src/jspropertycache.cpp
@@ +155,5 @@
>               * N.B. Objects are not branded if type inference is enabled, to
>               * allow property accesses without shape checks in JIT code.
>               */
> +            if (false && //XXX: disable branding on the IM branch (branding will be removed soon)
> +                !pobj->generic() && shape->hasDefaultGetter() && pobj->containsSlot(shape->slot) &&

You can probably omit this since without TI we won't really be optimizing SETGNAME anyway.
Comment 4 David Anderson [:dvander] 2011-11-07 14:55:04 PST
Comment on attachment 572594 [details] [diff] [review]
Patch v1

Brian should review the TI integration stuff, I'm still learning there.
Comment 5 David Anderson [:dvander] 2011-11-07 14:56:57 PST
> You can probably omit this since without TI we won't really be optimizing SETGNAME anyway.

Err, sorry, that's clearly not true. Disabling branding seems fine, avoids a shape guard.
Comment 6 David Anderson [:dvander] 2011-11-07 15:02:24 PST
(In reply to David Anderson [:dvander] from comment #5)
> > You can probably omit this since without TI we won't really be optimizing SETGNAME anyway.
> 
> Err, sorry, that's clearly not true. Disabling branding seems fine, avoids a
> shape guard.

Err... and, I'm wrong again. We need a shape guard anyway, a non-configurable, writable property can actually be reconfigured as readonly. I'm not sure which TI check guards against this, but that's what would be needed to elide the guard.
Comment 7 Brian Hackett (:bhackett) 2011-11-07 23:16:28 PST
Comment on attachment 572594 [details] [diff] [review]
Patch v1

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

::: js/src/ion/IonBuilder.cpp
@@ +2187,5 @@
> +        return abort("SETGNAME property has no slot");
> +
> +    if (propertyTypes && propertyTypes->isOwnProperty(cx, globalObj->getType(cx), true))
> +        return abort("SETGNAME property is non-configurable, non-enumerable or non-writable");
> +

Re: isOwnProperty comment, talked with jandem on IRC about this, when the second argument to isOwnProperty is 'true' then it is checking for a configurable property (where maybe-configurable properties are also maybe-own-properties).  These should be split out into separate interface methods for clarity.

@@ +2193,5 @@
> +    current->add(global);
> +
> +    if (shape->configurable()) {
> +        MGuardShape *guard = MGuardShape::New(global, shape);
> +        current->add(guard);

Will this always insert a shape guard on the global?  Global shape guards should never be necessary, given the isOwnProperty call (which will trigger recompilation should the property be deleted or otherwise reconfigured).
Comment 8 Jan de Mooij [:jandem] 2011-11-08 00:09:02 PST
(In reply to Brian Hackett from comment #7)
> Will this always insert a shape guard on the global?  Global shape guards
> should never be necessary, given the isOwnProperty call (which will trigger
> recompilation should the property be deleted or otherwise reconfigured).

Globals declared using "var" are non-configurable. However, as David pointed out in comment 6, a non-configurable property can still be made non-writable (I thought non-configurable prevented this, will add a testcase). This means we'll always have to add a shape guard without TI.

I didn't want to get rid of the shape guard with TI, because we don't have on stack invalidation. However, this is not a very good reason, and it seems better now to add a shape guard iff propertyTypes is null (eg. TI is disabled).
Comment 9 Jan de Mooij [:jandem] 2011-11-08 00:26:49 PST
(In reply to David Anderson [:dvander] from comment #3)
> We should support embedding
> non-double constants in StoreSlotT, at least on x86/x64.

Good idea, will add this today.

> Maybe "slots" instead of "to"? (Not sure what MLoadSlot has, but if it has
> "from", slots sounds better there too.)

Yeah MLoadSlot has "from", I will change them both to "slots'.
Comment 10 Jan de Mooij [:jandem] 2011-11-08 05:08:07 PST
Created attachment 572784 [details] [diff] [review]
Patch v2

Addresses all review comments. I had to leave branding disabled though; jit-test jaeger/bug627486.js still fails even though we now always add the shape guard (without TI).

I noticed the shape guard for getgname/setgname is too restricted; we guard on the property shape, not globalObj->shape(). It's probably best to wait for ObjShrink, it will remove shape numbers.
Comment 11 David Anderson [:dvander] 2011-11-08 15:29:40 PST
Comment on attachment 572784 [details] [diff] [review]
Patch v2

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

::: js/src/ion/x64/CodeGenerator-x64.cpp
@@ +278,5 @@
> +        masm.movq(ScratchReg, Operand(base, offset));
> +    } else {
> +        JSValueShiftedTag tag = MIRTypeToShiftedTag(valueType);
> +        masm.boxValue(tag, ToOperand(value), ScratchReg);
> +        masm.movq(ScratchReg, Operand(base, offset));

The IR design was sort of motivated to avoid this pattern. For example, the box instruction could be in MIR, where it could participate in optimizations, and then we could peephole past Box instructions if it made a difference on certain architectures.

That adds a lot to the compiler pipeline though (RA would need to eliminate dead LIR, and we'd have way more box instructions everywhere). Another idea would be to introduce a TypePolicy with architecture snooping, so it could box certain typed inputs on x64.

Anyway, that's just food for thought, the code here is fine.
Comment 12 Jan de Mooij [:jandem] 2011-11-11 23:30:38 PST
*** Bug 701954 has been marked as a duplicate of this bug. ***
Comment 13 David Anderson [:dvander] 2011-11-11 23:35:37 PST
Sorry, I forgot to reply to some earlier comments.

 (1) Luke would have a good idea of what the compile-n-go plan is. I think we should
     check c-n-g bits on a per-opcode basis, but for now it's fine to abort if it's not
     set.

 (2) Yeah, the shape guarding is bogus. I don't know whether it's worth fixing this, given
     that ObjShrink is on the horizon and we don't have invalidation yet.

 (3) This patch sort of forces us to think about the write barriers :) I'll file a
     follow-up bug.
Comment 14 Jan de Mooij [:jandem] 2011-11-12 01:04:52 PST
http://hg.mozilla.org/projects/ionmonkey/rev/535a0dda3328

Filed bug 701993 for the ARM part.

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