Closed
Bug 700303
Opened 13 years ago
Closed 13 years ago
IonMonkey: Compile JSOP_SETGNAME
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
29.16 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
Patch is almost done but I want to test this a bit more before asking for review.
Assignee | ||
Comment 1•13 years ago
|
||
Not sure about some things like the globalPropertyWrite function, suggestions would be very welcome.
Attachment #572594 -
Flags: review?(dvander)
Assignee | ||
Comment 2•13 years ago
|
||
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 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.
Attachment #572594 -
Flags: review?(dvander) → review+
Comment on attachment 572594 [details] [diff] [review] Patch v1 Brian should review the TI integration stuff, I'm still learning there.
Attachment #572594 -
Flags: review?(bhackett1024)
> 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.
(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•13 years ago
|
||
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).
Attachment #572594 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 8•13 years ago
|
||
(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).
Assignee | ||
Comment 9•13 years ago
|
||
(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'.
Assignee | ||
Comment 10•13 years ago
|
||
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.
Attachment #572594 -
Attachment is obsolete: true
Attachment #572784 -
Flags: review?(dvander)
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.
Attachment #572784 -
Flags: review?(dvander) → review+
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.
Assignee | ||
Comment 14•13 years ago
|
||
http://hg.mozilla.org/projects/ionmonkey/rev/535a0dda3328 Filed bug 701993 for the ARM part.
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•