Closed Bug 707854 Opened 8 years ago Closed 8 years ago

IonMonkey: Implement ICs for JSOP_GETPROP


(Core :: JavaScript Engine, defect)

Not set





(Reporter: dvander, Assigned: bhackett)


(Blocks 1 open bug)



(1 file, 4 obsolete files)

Now that we're starting to benchmark and add more complicated ops, Brian would like to start prototyping ICs to see see how they fit into the architecture. More here soon, but our initial thoughts:

The major goal over JM is simplicity, since ICs were very, very error-prone. That means eliminating inline paths and making sure we can safely handle re-entrancy and GC issues.

We also have a chance to design better type-safe structures for creating, holding, and patching code locations. (For example, we should solve the x64 patching problem up-front, which should be easy since the assembler already supports it.)
Attached patch crude first cut (obsolete) — Splinter Review
Crude patch that does the right thing on a simple example.  Each access site that is an IC is initially compiled as a jump to an OOL snippet of code that calls an IC function, which attaches stubs in a chain to the original jump.
Assignee: general → bhackett1024
Attached patch WIP (obsolete) — Splinter Review
Cleaned up WIP, can read unboxed or boxed values out of native objects and works on x86 and x64 (leveraging the assembler's far jump table to allow patching near jumps into far jumps).  Still some missing pieces and some things I need understand better about the infrastructure.
Attachment #580611 - Attachment is obsolete: true
Attached patch patch (351e94bf59cb) (obsolete) — Splinter Review
IC for GETPROP.  This works, and is about as far as I can get with the current state of things.  This won't be totally correct until on stack invalidation is in place, and once that is done will need followup work to allow making calls from within generated stubs, and to allow idempotent caches.
Attachment #580794 - Attachment is obsolete: true
Attachment #581061 - Flags: review?(dvander)
Attached patch patch (5ee070c3f2d0) (obsolete) — Splinter Review
Replace uses of Operand in shared code with AnyRegister, move ValueOperand to IonRegisters.h, some other cleanup.  After this lands, will rename ValueOperand to ValueRegisters, there are a lot of hits of VAlueOperand and need to minimize diff size.
Attachment #581061 - Attachment is obsolete: true
Attachment #581061 - Flags: review?(dvander)
Attachment #581487 - Flags: review?(dvander)
Review ping.
Comment on attachment 581487 [details] [diff] [review]
patch (5ee070c3f2d0)

Review of attachment 581487 [details] [diff] [review]:

Sorry for the late review. I really like the new design for the most part, and I just have style/factoring comments. My only concern is storing absolute addresses in the IC structures. Is that necessary? It might be a potential new source of complexity. Could we store relative locations instead, and only grab the absolute labels when it comes time to patch?

::: js/src/ion/CodeGenerator.cpp
@@ +535,5 @@
> +    return true;
> +}
> +
> +bool
> +CodeGenerator::visitOutOfLineGetPropertyCache(OutOfLineGetPropertyCache *ool)

I strongly prefer this split into two separate functions, since most of the logic is branched. It'd be a lot easier to read with the surrounding bits duplicated. There might be other ways to factor it.

::: js/src/ion/IonCaches.cpp
@@ +64,5 @@
> +    markAbsolute(true);
> +}
> +
> +static inline void
> +PatchJump(CodeLocationJump jump, CodeLocationLabel label)

Please move this logic into Assembler-$(ARCH), all of these details should ideally be completely hidden. This might necessitate moving the CodeLocation/Offset headers into Assembler-shared.h, which is okay.

@@ +129,5 @@
> +    }
> +
> +    Label rejoin_;
> +    CodeOffsetJump rejoinOffset = masm.jumpWithPatch(&rejoin_);
> +    masm.bind(&rejoin_);

Is this bind just so the destructor won't complain? Hrm.. that's a little sad, but I don't have any good ideas on how to fix it. I'll think about it more tomorrow.

::: js/src/ion/IonCaches.h
@@ +75,5 @@
> +#endif
> +
> +    CodeOffsetJump() { PodZero(this); }
> +
> +    size_t offset() const { return offset_; }

Extreme nit: Local style in ion/* is to expand these to multiple lines.

::: js/src/ion/IonFrames.h
@@ +322,5 @@
> +    ++iter;
> +    JS_ASSERT(iter.type() == IonFrame_JS);
> +    IonJSFrameLayout *frame = static_cast<IonJSFrameLayout*>(iter.current());
> +    JSFunction *fun = CalleeTokenToFunction(frame->calleeToken());
> +    return fun->script()->ion;

If this necessitated the vm/Stack.h change, could these functions be moved to an IonFrameIterator-inl.h? (Also, naming nit: GetTopIonScript)

::: js/src/ion/IonMacroAssembler.cpp
@@ +104,5 @@
> +void
> +MacroAssembler::PushVolatileRegsInMask(RegisterSet set)
> +{
> +    AnyRegisterVector regs;
> +    FillRegisterVector(set, regs);

Why not use AnyRegisterIterator?

@@ +112,5 @@
> +        if (!regs[i].volatile_())
> +            continue;
> +        if (regs[i].isFloat()) {
> +            storeDouble(regs[i].fpu(),
> +                        Address(StackPointer, -diff - sizeof(double)));

Is it safe to write below the stack - can an interrupt or signal clobber it? I vaguely suspect the answer is "no" but Valgrind will probably complain about it.

@@ +123,5 @@
> +    }
> +
> +    subPtr(Imm32(diff), StackPointer);
> +
> +    framePushed_ += diff;

You can call reserveStack() here.

@@ +130,5 @@
> +void
> +MacroAssembler::PopVolatileRegsInMask(RegisterSet set)
> +{
> +    AnyRegisterVector regs;
> +    FillRegisterVector(set, regs);

Ah - that's why. Having a reverse register iterator would be nicer, but not needed for this patch.

@@ +147,5 @@
> +    }
> +
> +    addPtr(Imm32(diff), StackPointer);
> +
> +    framePushed_ -= diff;

You can call freeStack() here.

::: js/src/ion/IonMacroAssembler.h
@@ +162,5 @@
> +        return CodeOffsetLabel(size());
> +    }
> +
> +    void PushVolatileRegsInMask(RegisterSet set);
> +    void PopVolatileRegsInMask(RegisterSet set);

(Note, the "Volatile" bit might not be long-lived: After bug 695075, we'll push the exact live registers, and note the subset of those that hold gcthings, which could be non-volatile regs.)

::: js/src/ion/IonSpewer.h
@@ +71,5 @@
>      _(Bailouts)                             \
>      /* Debug info about snapshots */        \
> +    _(Snapshots)                            \
> +    /* Generated inline cache stubs */      \
> +    _(InlineCaches)

Super small nit: s/InlineCaches/Caches/ to match the env flag. Shorter to type too.

::: js/src/ion/MOpcodes.h
@@ +84,5 @@
>      _(Elements)                                                             \
>      _(LoadSlot)                                                             \
>      _(StoreSlot)                                                            \
>      _(TypeBarrier)                                                          \
> +    _(GetPropertyCache)                                                     \

Right now we also have "LoadSlot", "LoadElement" etc. We should stick with the "Load" theme or change the others to "Get". (Super-small nit, I like "IC" better than "Cache", but it's up to you.)

::: js/src/ion/shared/CodeGenerator-shared-inl.h
@@ +111,5 @@
> +    // XXX it would be nice to be able to use outputType and outputPayload here.
> +    return ValueOperand(ToRegister(ins->getDef(TYPE_INDEX)),
> +                        ToRegister(ins->getDef(PAYLOAD_INDEX)));
> +#elif defined(JS_PUNBOX64)
> +    // XXX it would be nice to be able to use outputValue here.

Can you remove these // XXX lines? Trying to discourage that and :TODO: since they light up vim and usually no one ever fixes them :)

::: js/src/ion/x64/MacroAssembler-x64.h
@@ +466,5 @@
> +    void unboxValue(const ValueOperand &src, AnyRegister dest) {
> +        if (dest.isFloat()) {
> +            Label notInt32, end;
> +            branchTestInt32(Assembler::NotEqual, src, &notInt32);
> +            breakpoint();

breakpoint intended?

::: js/src/vm/Stack.h
@@ +41,5 @@
>  #ifndef Stack_h__
>  #define Stack_h__
>  #include "jsfun.h"
> +#include "ion/IonFrames.h"

Luke specifically asked me not to include IonFrames.h here, since it takes in a lot of random Ion stuff. Can this be switched back to IonFrameIterator.h?
Attachment #581487 - Flags: review?(dvander)
Updated patch.

I refactored the OutOfLineGetPropertyCache stuff to do less branching.  The branching was kind of ugly, yeah, but I think in the long run maintenance, refactoring and other changes will be much harder to get right if we tolerate duplicated code.

I think that the Load and Get operations are doing distinctly different things, and having different names helps with understanding rather than hurts it.  Loading is a low level memory operation --- get this index from that value array --- and getting is a high level language operation.  If there is a scripted getter then the GetPropertyCache will end up invoking a function rather than loading a slot, and in some cases we will be able to resolve caches without doing any memory ops at all.

I also removed the register vector and made all stores of volatile regs under the stack pointer, which requires another congruent loop but is cleaner overall.
Attachment #581487 - Attachment is obsolete: true
Attachment #583158 - Flags: review?(dvander)
Comment on attachment 583158 [details] [diff] [review]
patch (bf524b56351f)

Review of attachment 583158 [details] [diff] [review]:

r=me with the IonFrames.h dependency removed from vm/Stack.h. Feel free to make an IonFrameIterator-inl.h if needed.

(I would like to deal with the Label binding issue, but I don't want to hold up this patch on it.)
Attachment #583158 - Flags: review?(dvander) → review+
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 716743
You need to log in before you can comment on or make changes to this bug.