805877 - (BaselineOSR) BaselineCompiler: Implement OSR between Baseline <=> Ion

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

Add OSR support to baseline compiler.  This is useful because there is a lot of code which runs exactly once or exactly twice.  It's probably not worthwhile to compile these and better to just run them via interpreter.

We can compile on 3rd or 4th or 5th run or whatever else makes sense, but we need a way to enter JITcode from the interpreter on loop edges.  This will probably just involve a trampoline that sets up the stack and jumps into the middle of the baseline jitscript.

I don't think the main script code in the baseline itself needs to be aware of whether the entry was via OSR or through regular call - the OSR entry trampoline can re-create the stack/registers as would be expected by the script, since the stack frame structure is so simple.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: Baseline to Ion OSR

No rush on this review, Jan - get to it when you have time.  I'm still working on the bailout code, and we can't push any of this before bailouts work.

Give this a once-over whenever you get a chance and see if there's any major issues you see with the approach.

Comment 2

[Kannan Vijayan [:djvj]]

Attachment: Marking of Baseline->Ion OSR frames

Same as above: no rush.  Take a look if/when you have the interest.  I just wanted to make sure that my WIP so far was available for you to look at before I took off for the holidays.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 695492 [details] [diff] [review]
Baseline to Ion OSR

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

Nice, this definitely seems simpler/faster than going back to the interpreter to enter Ion. Reusing the StackFrame layout is clever, avoids two separate Ion OSR mechanisms.

I thought about this more over the holidays, and I think it would be good to have the C++ stub malloc a buffer for the fake StackFrame, fill it in and have it return a pointer to this buffer (we can also store the Ion-jitcode pointer in it somewhere). Constructing the fake StackFrame in C++ is simpler and more maintainable, and it means we can do real on-stack "replacement": after calling the C++ stub, all we have to do is pop the BaselineFrame (including locals/stack values) and jump into Ion. This also avoids the need for the OSR frame type since we just transform the baseline frame into an optimized frame.

The downside is that we have to |free| this fake StackFrame somewhere. To do this, we could add a new instruction (MFreeOsrBuffer or something) to the end of the OSR block, which checks if it's a fake StackFrame and if it is, performs a callWithABI to js_free. None of the instructions inside the OSR block should bailout, but if we are worried about that we can always add a debug-only flag to JSRuntime which we set to |true| when allocating the buffer and reset to |false| when we free it, and assert it's |false| in some strategic places.

Comment 4

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

Could we just copy the C++ buffer to the stack and free it immediately?

Comment 5

[Kannan Vijayan [:djvj]]

Copying it to the stack would mean we'd still have to have a separate frame type, no?

Comment 6

[Kannan Vijayan [:djvj]]

dvander: I thought more about this over the last couple days, and what you suggested seems like it should be doable.  The reason I thought it required a separate frame type was because I assumed that the fake StackFrame would have to be traced for GC.  However, Jandem let me know that the ion OSREntry basic block will never cause a GC, which means that we don't have to worry about this.

Tell me if the following makes sense.

Initially, we have the following stack state:


  [...prev-frame...] [ACTUAL-ARGS] [CALLEE] [DESCR] [BaselineJS-frame]
  |                                       |
  |                                       |
  \---------------------------------------/
            DESCR's size


When we OSR into, Ion, we enter a C++ function that:
1. Allocates heapspace to hold [FORMAL-ARGS, StackFrame, LOCALS], as well as a copy of ACTUAL-ARGS taken from the current BaselineJS frame.

2. Creates a fake StackFrame within the heapspace, and copies ACTUAL-ARGS into heap too.

3. Returns to JITcode passing a pointer to this information.

The JITcode takes this information and copies it back onto the stack, replacing the existing BaselineJS frame with an Ion JS frame as follows:

  [...prev-frame...] [StackFrame] [ACTUAL-ARGS] [CALLEE] [DESCR] [OptimizedJS-frame]
  |                                                    |
  |                                                    |
  \----------------------------------------------------/
                      DESCR's size



Basically, the StackFrame is stuck in between the previous frame and the Ion frame, with neither the previous frame or the ion frame being aware of its existence.  It's just incorporated into the frame descriptor's size.


Jandem is not sure if it's OK to do this (have "hidden" information stowed in between the previous frame and the ion frame), especially in the case where the previous frame is also an Ion frame.

Will doing this cause any problems?

Comment 7

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

Nope, I don't think there are any problems with that. You'll be copying an extra time but I don't think it matters. As long as frame iteration knows to skip over the dead area you should be good.

Where does the StackFrame heap space get freed?

Comment 8

[Kannan Vijayan [:djvj]]

It'll get freed by the transition assembly code once it's been copied onto the stack, before we enter Ion - pretty much what you suggested.

Comment 9

[Kannan Vijayan [:djvj]]

Comment on attachment 695493 [details] [diff] [review]
Marking of Baseline->Ion OSR frames

This patch is irrelevant.  We shouldn't need to mark the fake StackFrame because GC will never run in the period when it's accessed.

Comment 10

[Kannan Vijayan [:djvj]]

Attachment: Baseline to Ion OSR, try 2

New patch for baseline to Ion OSR, based on suggestions.

The new strategy, summarized:

1. UseCount IC fallback C++ function is hit, determines that we want to try to OSR into ion.
2. Ion compile is attempted, and if it fails, then useCount is reset to zero and.
3. If Ion compile succeeds, then some heap memory is allocated to store both a copy of the current baseline frame, and a fake interpreter StackFrame for use with Ion's osr entry jitcode.
4. The C++ function returns to a bit of stubcode that copies this heap info back onto the stack, replacing the existing baseline frame in the process.
5. The stubcode then frees the heap memory, and JUMPS (not calls) to the ion jitcode (since the return address has already been "pushed").

This patch doesn't build when applied to the ionmonkey tree tip because some of the macro assembler methods it depends on have been checked into mozilla-inbound tree.  It should build after those changes have been merged into the ionmonkey tree.

Comment 11

[Kannan Vijayan [:djvj]]

Note: see the big fat comment in BaselineIC.cpp, which describes the layout of stuff that's copied and how.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 701222 [details] [diff] [review]
Baseline to Ion OSR, try 2

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

Looks good! Mostly style nits, but I think a final review with these comments addressed would be good, since it's one of the most complicated parts of the compiler.

::: js/src/ion/BaselineIC.cpp
@@ +300,5 @@
>  //
>  // UseCount_Fallback
>  //
>  
> + static bool

Nit: whitespace before |static|

@@ +323,5 @@
> +        IonSpew(IonSpew_BaselineICFallback, "  IonScript exists, and is compiling off thread!");
> +        // TODO: reduce useCount by a small amount here?  We don't want to reset the useCount
> +        // since we expect the compiled script to be ready soon.  But we also don't want
> +        // to poll the compilation by hitting this fallback stub every time.
> +        script->resetUseCount();

JM does the following if off-thread compilation is enabled: call a stub if (a) use count >= threshold and (b) script->ion is NULL. When off-thread compilation finishes, it patches the check for (a) to skip (b).

With our IC design we don't have to patch anything and we can do something simpler. When we kick off an off-thread compilation, attach a stub that's just a no-op if script->ion != NULL. Else (script->ion == NULL) it jumps to the fallback stub (this happens when off-thread compilation was interrupted). When off-thread compilation finishes, all we have to do is remove this stub from the chain (near the call to DisableScriptCodeForIon in Ion.cpp), so that the next time the use count >= threshold we will call into the VM again and can perform OSR.

We can do this as a follow-up though if you file a bug.

@@ +328,5 @@
> +        return true;
> +    }
> +
> +    // If this script is already ion-compiled
> +    if (script->hasIonScript()) {

Can we just always call CanEnterAtBranch here? The code below doesn't handle the bailoutExpected flag, and all these heuristics can be fragile so it would be good to have them in one place and match the interpreter OSR.

@@ +466,5 @@
> +    size_t totalSpace = AlignBytes(actualArgsSpace, sizeof(Value)) +
> +                        AlignBytes(stackFrameSpace, sizeof(Value)) +
> +                        AlignBytes(ionOsrTempDataSpace, sizeof(Value));
> +
> +    uint8_t *tempInfo = (uint8_t *) js_malloc(totalSpace);

js_calloc zero initializes, it's a bit safer and helps debugging.

@@ +508,5 @@
> +    // low to high addresses, while on the C stack, they go from high to low addresses.
> +    // So we can't use memcpy on this, but must copy the values in reverse order.
> +    Value *stackFrameLocalsStart = (Value *) (stackFrame + sizeof(StackFrame));
> +    for (size_t i = 0; i < numLocalsAndStackVals; i++)
> +        stackFrameLocalsStart[i] = *(((Value *) baselineFrame) - (i + 1));

Can you add a method "Value slotValue(size_t )" to BaselineFrame and use it here?

@@ +541,5 @@
> +    void *jitcode = NULL;
> +    bool canEnterIon = false;
> +    if (!EnsureCanEnterIon(cx, stub, frame, script, pc, &canEnterIon, &jitcode))
> +        return false;
> +    if (!canEnterIon)

You can make this "if (!jitcode)" and remove the canEnterIon argument.

@@ +549,5 @@
> +    IonOsrTempData *info = PrepareOsrTempData(cx, stub, frame, script, pc, jitcode);
> +    if (!info)
> +        return false;
> +    *infoPtr = info;
> +    

Nit: whitespace

@@ +570,5 @@
> +static const VMFunction FreeOsrTempDataInfo =
> +    FunctionInfo<FreeOsrTempDataFn>(FreeOsrTempData);
> +
> +static bool
> +GenerateCopy(MacroAssembler &masm, Register copyFrom, Register copyEnd, Register copyTo,

Would be good to move this to IonMacroAssembler.h (and change the return type to void).

@@ +635,5 @@
> +    masm.loadPtr(Address(osrDataReg, offsetof(IonOsrTempData, stackFrameEnd)), copyEnd);
> +    masm.movePtr(BaselineStackReg, copyTo);
> +    if (!GenerateCopy(masm, copyFrom, copyEnd, copyTo, tempReg))
> +        return false;
> +    

Nit: some whitespace here

@@ +657,5 @@
> +    masm.movePtr(reg0, reg1);
> +    masm.rshiftPtr(Imm32(FRAMESIZE_SHIFT), reg1);
> +    masm.addPtr(Address(osrDataReg, offsetof(IonOsrTempData, stackFrameSize)), reg1);
> +    masm.lshiftPtr(Imm32(FRAMESIZE_SHIFT), reg1);
> +    masm.andPtr(Imm32((1 << FRAMESIZE_SHIFT) - 1), reg0);

Nit: Imm32(FRAMETYPE_MASK)

@@ +658,5 @@
> +    masm.rshiftPtr(Imm32(FRAMESIZE_SHIFT), reg1);
> +    masm.addPtr(Address(osrDataReg, offsetof(IonOsrTempData, stackFrameSize)), reg1);
> +    masm.lshiftPtr(Imm32(FRAMESIZE_SHIFT), reg1);
> +    masm.andPtr(Imm32((1 << FRAMESIZE_SHIFT) - 1), reg0);
> +    masm.addPtr(reg1, reg0);

Nit: orPtr

@@ +691,5 @@
> +
> +        if (!callVM(DoUseCountFallbackInfo, masm))
> +            return false;
> +
> +        // Otherwise, pop the stored 

Nit: "Otherwise, pop the stored frame register."

@@ +695,5 @@
> +        // Otherwise, pop the stored 
> +        masm.pop(R0.scratchReg());
> +
> +        // If no IonCode was found, then skip just exit the IC.
> +        masm.branchPtr(Assembler::Equal, R0.scratchReg(), ImmWord((uintptr_t) 0), &noCompiledCode);

Nit: ImmWord(NULL)

@@ +698,5 @@
> +        // If no IonCode was found, then skip just exit the IC.
> +        masm.branchPtr(Assembler::Equal, R0.scratchReg(), ImmWord((uintptr_t) 0), &noCompiledCode);
> +    }
> +
> +    // Prepare a RegisterSet to use for later code.  BaselineTailCallReg and

Nit: comment is not finished.

@@ +735,5 @@
> +    masm.push(scratchReg);
> +
> +    // Do the VM-call to free osr temp memory.  This call will never return false and will never GC.
> +    masm.push(osrDataReg);
> +    if (!callVM(FreeOsrTempDataInfo, masm))

This can be a masm.callWithABI.

::: js/src/ion/BaselineJIT.h
@@ +44,5 @@
>  
>      static ICStubSpace *FallbackStubSpaceFor(JSScript *script);
>      static ICStubSpace *StubSpaceFor(JSScript *script);
>  };
> + 

Nit: whitespace

::: js/src/ion/Ion.h
@@ +251,5 @@
>  bool SetIonContext(IonContext *ctx);
>  
>  bool CanIonCompileScript(JSContext *cx, UnrootedScript script);
>  
> +class StackFrameInfo

You can now make CanEnterAtBranch take a TaggedFramePtr (will be renamed to AbstractFramePtr soon) instead, it either points to a StackFrame or baseline frame. The baseline frame part is not yet implemented, but will be soon. (Would be good to make this change on m-c first to avoid conflicts).

The only method CanEnterAtBranch needs that's not trivial to implement for baseline frames is isConstructing (needs a StackIter), but the only place IonBuilder uses that is for GetPICSingleShape. I think you can just remove it from CompileInfo and have GetPICSingleShape handle both constructing and not-constructing.

Comment 13

[Kannan Vijayan [:djvj]]

Attachment: Expand PC mappings to more instructions and to include top-of-stack descriptor.

Comment 14

[Kannan Vijayan [:djvj]]

Attachment: Bailout from Ion to Baseline

The best place to start looking at this with:

ion/Bailouts.cpp.  |Bailout| and |InvalidationBailout| here have been expanded to call functions in BaselineJIT.cpp when restoring to baseline.

ion/BaselineJIT.cpp - the main meat of reconstructing stack frames.  Uses a helper class to manage a growable heap buffer where reconstruct into, as well as calculating virtual stack addresses.

ion/IonMacroAssembler.cpp - generateBailoutTail() gets modified to handle a new return value from Bailout or InvalidationBailout - reconstruct a baseline stack, and return into it.

There's some code in here which can be simplified.  Specifically, if we're rejoining with resumeAfter on a monitored op, then the bailout code makes sure to rejoin the baseline jit at the point where it enters the TypeMonitor IC for that op, to make sure that any necessary new stub gets added.  As dvander noted on IRC, this can just be done manually in the C++ code and the jitcode resumed at the beginning of the next op.  That should marginaly simplify some of the assembly code in generateBailoutTail.

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 705933 [details] [diff] [review]
Expand PC mappings to more instructions and to include top-of-stack descriptor.

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

::: js/src/ion/BaselineCompiler.cpp
@@ +377,5 @@
> +    // Main loop assumes that any given op can be bailed out on or
> +    // after.  If this is not the case, the op's handler can
> +    // set the fields to false.
> +    canResumeHere_ = true;
> +    canResumeAfter_ = true;

As discussed on IRC, you can handle the resumeAt case by adding an entry at every jumpTarget op (and the first op in the script), so that we only need canResumeAfter_.

@@ +436,5 @@
> +        //  2. Baseline can be resumed after the previous op, and the previous
> +        //     op does not have its result monitored (in that case, it'll resume
> +        //     directly into the TypeMonitor IC chain for the IC at that location.
> +        if (canResumeHere_ || (canResumeAfterPrevious && !previousOpMonitored)) {
> +            if (!addPCMappingEntry(opNativeOffset, opSlotInfo))

Can we do this earlier, after the debugMode_ |if|? At first I didn't notice the opNativeOffset and thought it used the offset after the op instead of before.

@@ +440,5 @@
> +            if (!addPCMappingEntry(opNativeOffset, opSlotInfo))
> +                return Method_Error;
> +        }
> +        canResumeAfterPrevious = canResumeAfter_;
> +        previousOpMonitored = !!(js_CodeSpec[op].format & JOF_TYPESET);

If we do the monitoring in C++, can we remove previousOpMonitored?

::: js/src/ion/BaselineFrameInfo.cpp
@@ +70,5 @@
> +{
> +    // Start at the bottom, find the first value that's not synced.
> +    uint32_t i = 0;
> +    for (; i < stackDepth(); i++) {
> +        if (stack[i].kind() == StackValue::Stack)

I think this should start at the top: peek(-(i + 1))->kind()

(peek(-1) gets the top value)

@@ +73,5 @@
> +    for (; i < stackDepth(); i++) {
> +        if (stack[i].kind() == StackValue::Stack)
> +            break;
> +    }
> +    return static_cast<int>(i);

Nit: make the return type uint32_t.

::: js/src/ion/BaselineJIT.h
@@ +84,3 @@
>      uint32_t pcOffset;
>      uint32_t nativeOffset;
> +    uint8_t slotInfo;

Nit: add a short comment here explaining the encoding

::: js/src/ion/shared/BaselineCompiler-shared.h
@@ +109,5 @@
> +        entry.slotInfo = slotInfo;
> +
> +        IonSpew(IonSpew_BaselineScripts, "PCMapping (%s:%d): %d => %d (%d:%d:%d)!",
> +                        script->filename, script->lineno,
> +                        (int) entry.pcOffset,

Nit: if you use %u instead of %d you can probably get rid of the (int) casts.

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 705947 [details] [diff] [review]
Bailout from Ion to Baseline

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

Nice! Bailing out from one JIT to another is no easy feat, but nice how the builder class hides most of the low-level complexity.

::: js/src/ion/BaselineJIT.cpp
@@ +734,5 @@
> +        //      STACK_START_ADDR + IonJSFrameLayout::Size() + PREV_FRAME_SIZE
> +        //                      - IonBaselineStubFrameLayout::reverseOffsetOfSavedFramePtr()
> +        if (type == IonFrame_BaselineStub) {
> +            size_t offset = IonJSFrameLayout::Size() + topFrame->prevFrameLocalSize() +
> +                            IonBaselineStubFrameLayout::reverseOffsetOfSavedFramePtr();

Shouldn't this be offsetOfSavedFramePtr, since we have a pointer to the start of BaselineStub layout?

@@ +754,5 @@
> +            return NULL;
> +
> +        // Otherwise, the frame preceding the rectifier is a BaselineStub frame.
> +        size_t extraOffset = IonRectifierFrameLayout::Size() + priorFrame->prevFrameLocalSize() +
> +                             IonBaselineStubFrameLayout::reverseOffsetOfSavedFramePtr();

Same here.

@@ +885,5 @@
> +    // Initialize BaselineFrame::frameSize
> +    size_t frameSize = BaselineFrame::Size() + BaselineFrame::FramePointerOffset +
> +                       (sizeof(Value) * (script->nfixed + exprStackSlots));
> +    IonSpew(IonSpew_BaselineBailouts, "      FrameSize=%d", (int) frameSize);
> +    *(blFrame->addressOfFrameSize()) = frameSize;

Nit: add a setFrameSize() method.

@@ +898,5 @@
> +        iter.skip();
> +    } else {
> +        Value v = iter.read();
> +        if (v.isObject())
> +            scopeChain = &(v.toObject());

Nit: no parentheses

@@ +904,5 @@
> +            JS_ASSERT(v.isUndefined());
> +        // TODO: check for heavyweight function? (fun->isHeavyweight())
> +    }
> +    // Get scope chain from function or script if not already set.
> +    if (!scopeChain) {

I think we should move this to the "else" branch above, the one with JS_ASSERT(v.isUndefined()). (If we failed the arguments check, we can leave scopeChain NULL so that the script prologue knows it still has to initialize it. This will only matter once we create call objects though.)

@@ +911,5 @@
> +        else
> +            scopeChain = &(script->global());
> +    }
> +    IonSpew(IonSpew_BaselineBailouts, "      ScopeChain=%p", scopeChain);
> +    *(blFrame->addressOfScopeChain()) = scopeChain;

Nit: blFrame->setScopeChain(scopeChain)

@@ +915,5 @@
> +    *(blFrame->addressOfScopeChain()) = scopeChain;
> +
> +    // Write undefined into scratch value.
> +    *(blFrame->addressOfScratchValue()) = UndefinedValue();
> +    *(blFrame->addressOfReturnValue()) = UndefinedValue();

Nit: we can leave these uninitialized, like the normal prologue does

@@ +925,5 @@
> +        IonSpew(IonSpew_BaselineBailouts, "      Is function!");
> +        IonSpew(IonSpew_BaselineBailouts, "      thisv=%016llx", *((uint64_t *) &thisv));
> +
> +        size_t thisvOffset = builder.framePushed() + IonJSFrameLayout::offsetOfThis();
> +        *((Value *) builder.pointerAtStackOffset(thisvOffset)) = thisv;

We could add a setThisValue() to BaselineFrame, like thisValue()

To avoid exposing everything and having to add setters, we can also move this code to BaselineFrame::initFromBailout. It would match StackFrame::initFromBailout and may be a bit more readable. What do you think?

@@ +936,5 @@
> +            Value arg = iter.read();
> +            IonSpew(IonSpew_BaselineBailouts, "      arg %d = %016llx",
> +                        (int) i, *((uint64_t *) &arg));
> +            size_t argOffset = builder.framePushed() + IonJSFrameLayout::offsetOfActualArg(i);
> +            *((Value *) builder.pointerAtStackOffset(argOffset)) = arg;

blFrame->formals()[i] = arg;

@@ +1025,5 @@
> +            // If needed, initialize BaselineBailoutInfo's valueR0 and/or valueR1 with the 
> +            // top stack values.
> +            uint8_t slotInfo = baselineScript->slotInfoForPC(script, pc);
> +            int numUnsynced = PCMappingEntry::SlotInfoNumUnsynced(slotInfo);
> +            JS_ASSERT(numUnsynced >= 0 && numUnsynced <= 2);

Nit: if you make numUnsynced uint32_t or unsigned, you don't need the >= 0 assert.

@@ +1046,5 @@
> +            // into registers.
> +            frameSize -= sizeof(Value) * numUnsynced;
> +            *(blFrame->addressOfFrameSize()) = frameSize;
> +            IonSpew(IonSpew_BaselineBailouts, "      Adjusted framesize -= %d: %d",
> +                            (int) (sizeof(Value) * numUnsynced), (int) frameSize);

Nit: I don't think you need the (int) casts, or else you can make them C++-style casts: int(frameSize).

@@ +1113,5 @@
> +    unsigned actualArgc = GET_ARGC(pc);
> +    JS_ASSERT(actualArgc + 2 <= exprStackSlots);
> +    for (unsigned i = 0; i < actualArgc + 1; i++) {
> +        size_t argOffset = (builder.framePushed() - endOfBaselineJSFrameStack) + (i*sizeof(Value));
> +        if (!builder.writeValue(*((Value *) builder.pointerAtStackOffset(argOffset)), "ArgVal"))

Maybe you can reuse the pointer to the frame here:

argSlot = numValueSlots - (actualArgc + 1) - 1;
for (...)
    writeValue(blFrame->valueSlot(argSlot));
    argSlotSlot++

@@ +1133,5 @@
> +    // Push callee token (must be a JS Function)
> +    uint32_t calleeStackSlot = exprStackSlots - uint32_t(actualArgc + 2);
> +    size_t calleeOffset = (builder.framePushed() - endOfBaselineJSFrameStack)
> +                            + ((exprStackSlots - (calleeStackSlot + 1)) * sizeof(Value));
> +    Value callee = *((Value *) builder.pointerAtStackOffset(calleeOffset));

Nit: add a builder.valueAtStackOffset() method

@@ +1242,5 @@
> +    //      OptimizedJS - Ion calling into Ion.
> +    //      BaselineStub - Baseline calling into Ion.
> +    //      Entry - Interpreter or other calling into Ion.
> +    //      Rectifier - Arguments rectifier calling into Ion.
> +    JS_ASSERT(iter.isScripted());

Nit: can this be isOptimizedJS()?

@@ +1269,5 @@
> +    //      |  ReturnAddr   |
> +    //      +---------------+
> +    //      |    |||||      | <---- Overwrite starting here.
> +    //      |    |||||      | 
> +    //      |    |||||      | 

Micro nit: some trailing whitespace here and on the previous line.

@@ +1291,5 @@
> +    RootedFunction callee(cx, iter.maybeCallee());
> +    if (callee) {
> +        IonSpew(IonSpew_BaselineBailouts, "  Callee function (%s:%u)",
> +                callee->nonLazyScript()->filename, callee->nonLazyScript()->lineno);
> +        // fp->formals()[-2].setObject(*callee);

Nit: remove comment.

@@ +1294,5 @@
> +                callee->nonLazyScript()->filename, callee->nonLazyScript()->lineno);
> +        // fp->formals()[-2].setObject(*callee);
> +    } else {
> +        IonSpew(IonSpew_BaselineBailouts, "  No callee!",
> +                callee->nonLazyScript()->filename, callee->nonLazyScript()->lineno);

Nit: remove the last two arguments.

::: js/src/ion/BaselineJIT.h
@@ +214,5 @@
>  EnterBaselineMethod(JSContext *cx, StackFrame *fp);
>  
> +struct BaselineBailoutInfo
> +{
> +    void *incomingStack;

Nit: can you add a short, one-line comment for most of these fields?

@@ +228,5 @@
> +};
> +
> +uint32_t
> +BailoutIonToBaseline(JSContext *cx, IonActivation *activation, IonBailoutIterator &iter,
> +                     bool invalidate, BaselineBailoutInfo **bailoutInfo);

Nit: move the actual bailout code to BaselineBailouts.cpp (does not need its own header file, but the cpp code is pretty self-contained)

::: js/src/ion/IonBuilder.cpp
@@ +199,5 @@
>          return false;
>      }
>  
> +    // Don't inline functions which don't have baseline scripts compiled for them.
> +    if (!inlineScript->hasBaselineScript())

Nit: add an IonSpew_Inlining message here.

::: js/src/ion/IonMacroAssembler.cpp
@@ +702,5 @@
> +            popValue(R0);
> +#if defined(JS_CPU_X86) || defined(JS_CPU_X64)
> +            jump(jitcodeReg);
> +#elif defined(JS_CPU_ARM)
> +            branch(jitcodeReg);

Nit: jump(jitcodeReg); should work on ARM as well.

Comment 17

[Kannan Vijayan [:djvj]]

(In reply to Jan de Mooij [:jandem] from comment #15)
> I think this should start at the top: peek(-(i + 1))->kind()
> 
> (peek(-1) gets the top value)

Good eye.  Serious error on my part.

> Can we do this earlier, after the debugMode_ |if|? At first I didn't notice the
> opNativeOffset and thought it used the offset after the op instead of before.

I think we need to do it before the debugMode_ |if|.  emitDebugTrap adds a PCMapping entry of its own before it emits any code.  I'll move the syncStack() in debug mode out of the emitDebugTrap() (and add an ASSERT there instead to ensure stack is synced) and into emitBody(), and make the pc mapping entry happen immediately befor the trap.

> Nit: if you use %u instead of %d you can probably get rid of the (int) casts.

Can I?  They're mostly uint32_t and uint8_t valued things, and I can't be sure whether the compiler will ensure they're int-sized before pushing, since we can't assume sizeof(int) == sizeof(int32).  %u would just force me to cast to unsigned int instead.

Comment 18

[Kannan Vijayan [:djvj]]

(In reply to Kannan Vijayan [:djvj] from comment #17)
> > Nit: if you use %u instead of %d you can probably get rid of the (int) casts.
> 
> Can I?  They're mostly uint32_t and uint8_t valued things, and I can't be
> sure whether the compiler will ensure they're int-sized before pushing,
> since we can't assume sizeof(int) == sizeof(int32).  %u would just force me
> to cast to unsigned int instead.

Just checked default argument promotion semantics for functions passed in variadic position.  Your suggestion should work fine.

Comment 19

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

Comment on attachment 701222 [details] [diff] [review]
Baseline to Ion OSR, try 2

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

::: js/src/ion/BaselineIC.cpp
@@ +466,5 @@
> +    size_t totalSpace = AlignBytes(actualArgsSpace, sizeof(Value)) +
> +                        AlignBytes(stackFrameSpace, sizeof(Value)) +
> +                        AlignBytes(ionOsrTempDataSpace, sizeof(Value));
> +
> +    uint8_t *tempInfo = (uint8_t *) js_malloc(totalSpace);

If you want to avoid having to js_free() somewhere, I think you can just reserve space on the contiguous stack, but not commit a new stack pointer. As long as nothing pushes an interpreter frame in between you're good.

::: js/src/ion/Ion.h
@@ +283,5 @@
> +    bool isFunctionFrame() { return fp->isFunctionFrame(); }
> +    bool isEvalFrame() { return fp->isEvalFrame(); }
> +    bool isGeneratorFrame() { return fp->isGeneratorFrame(); }
> +    bool isDebuggerFrame() { return fp->isDebuggerFrame(); }
> +    bool isAnnotated() { return fp->annotation() != NULL; }

nit: Ion style should be {
    return ..
}

Comment 20

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

Comment on attachment 705933 [details] [diff] [review]
Expand PC mappings to more instructions and to include top-of-stack descriptor.

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

::: js/src/ion/BaselineCompiler.cpp
@@ +835,5 @@
>  bool
>  BaselineCompiler::emit_JSOP_UINT16()
>  {
> +    canResumeHere_ = false;
> +    canResumeAfter_ = false;

These annotations are kind of gross. Possible to have an ahead-of-time static table?

Comment 21

[Kannan Vijayan [:djvj]]

Attachment: Expand PC mappings to include top-of-stack descriptor

Nits fixed, second stab.  As david suggested, I factored out the canResumeAfter_ into a static method.  We can make this into a table if we need to, but for now it's a giant switch statement within a static method.

Comment 22

[Kannan Vijayan [:djvj]]

Comment on attachment 706657 [details] [diff] [review]
Expand PC mappings to include top-of-stack descriptor

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

::: js/src/ion/BaselineJIT.cpp
@@ +30,5 @@
>  {
>      JS_ASSERT(script->hasBaselineScript());
>      return script->baselineScript()->optimizedStubSpace();
>  }
> + 

End-of-line whitespace here is fixed.  Missed that.

Comment 23

[Jan de Mooij [:jandem]]

Comment on attachment 706657 [details] [diff] [review]
Expand PC mappings to include top-of-stack descriptor

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

::: js/src/ion/BaselineCompiler.cpp
@@ +42,5 @@
>      return true;
>  }
>  
> +/*static*/ bool
> +BaselineCompiler::CanResumeAfter(JSOp op)

Nit: doesn't have to be a method of the compiler:

static bool
CanResumeAfter(..)

@@ +410,5 @@
>      JS_ASSERT(pc == script->code);
>  
> +    // Main loop assumes that any given op can be bailed out on or
> +    // after.  If this is not the case, the op's handler can
> +    // set the fields to false.

Nit: update comment.

@@ +443,5 @@
> +        // We need to add a PC -> native mapping entry for the upcoming op if one
> +        // of the following is true:
> +        //  1. The pc is a jumptarget.
> +        //  2. Baseline can be resumed after the previously handled op.
> +        //  3. Op is a JSOP_ENTERBLOCK

Nit: add something like ", for catch blocks"

Comment 24

[Kannan Vijayan [:djvj]]

Attachment: Baseline to Ion OSR, try 3

Comments addressed.  Changed to use AbstractFramePtr instead of custom frame abstraction.

Also, added a single line to disable the actual entry into ion.  This patch alone will cause lots of test failures since it doesn't include bailout code yet, and that's needed for successfully deoptimizing from Ion into baseline.  For that reason, there's a single line in BaselineIC.cpp (see FIXME) that disables actual entry into ion.  This line will be removed by patch for bailout.

Comment 25

[Jan de Mooij [:jandem]]

Comment on attachment 707174 [details] [diff] [review]
Baseline to Ion OSR, try 3

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

Great! r=me with comments below addressed.

::: js/src/ion/BaselineFrame.h
@@ +228,5 @@
>          return sizeof(BaselineFrame);
>      }
>  
> +    // Obtain a baseline frame pointer from the corresponding IonJSFrameLayout pointer.
> +    static BaselineFrame *FromIonJSFrame(IonJSFrameLayout *frame) {

Nit: don't need this function if you pass BaselineFrame * to the stub (see other comment)

@@ +257,5 @@
>      }
>      static size_t reverseOffsetOfLocal(size_t index) {
>          return -BaselineFrame::Size() - (index + 1) * sizeof(Value);
>      }
> +    static size_t reverseOffsetOfLocalsBegin() {

Nit: function is not used

::: js/src/ion/BaselineIC.cpp
@@ +324,5 @@
> +
> +    // If this script is compiling off thread, then wait for that to finish.
> +    if (script->isIonCompilingOffThread()) {
> +        IonSpew(IonSpew_BaselineOSR, "  IonScript exists, and is compiling off thread!");
> +        // TODO: reduce useCount by a small amount here?  We don't want to reset the useCount

Nit: can you file a follow-up bug for the suggestion in comment 12, so that we don't forget about it?

@@ +564,5 @@
>  static const VMFunction DoUseCountFallbackInfo =
>      FunctionInfo<DoUseCountFallbackFn>(DoUseCountFallback);
>  
> +static bool
> +FreeOsrTempData(JSContext *cx, void *data)

Nit: function is unused

@@ +575,5 @@
> +static const VMFunction FreeOsrTempDataInfo =
> +    FunctionInfo<FreeOsrTempDataFn>(FreeOsrTempData);
> +
> +static bool
> +GenerateCopy(MacroAssembler &masm, Register copyFrom, Register copyEnd, Register copyTo,

Nit: move to IonMacroAssembler.h/cpp, seems like it could be useful for other things.

@@ +687,5 @@
> +        masm.subPtr(Imm32(sizeof(void *)), BaselineStackReg);
> +        masm.push(BaselineStackReg);
> +
> +        // Push IonJSFrameLayout pointer.
> +        masm.addPtr(Imm32(BaselineFrame::FramePointerOffset), R0.scratchReg());

If you pass BaselineFrame * (masm.loadBaselineFramePtr(R0.scratchReg(), R0.scratchReg()), it would slightly simplify some (C++) code I think.

Comment 26

[Kannan Vijayan [:djvj]]

I'll check in the Baseline->Ion OSR and the PCMapping patches tomorrow.

In the meantime, I ran into a couple of issues today when fixing up the Ion->Baseline bailout code to pass tests.

It seems more difficult than initially estimated to do type monitoring from within the C++ code when bailing out from ion to baseline, because of the possibility of the following happening:

1. Ion bails on a type-monitored op, invoking the ion->baseline bailout code.
2. The baseline bailout code calls TypeMonitor.
3. TypeMonitor causes changes in the type system which triggers invalidation of IonCode.
4. Ion invalidation code attempts to scan the stack for active Ion frames.
5. Ion code invalidates because ionTop has been cleared at the start of the bailout code.

I thought about just not clearing ionTop on entry to bailouts, but I wasn't sure whether that was a good idea or not.  The C stack with the ion frames on it should still be fine at the point the type monitoring is done (since the reconstructed baseline stack is still in the allocated heap memory).  Technically, the stack scan by ion's script invalidation should go fine.

However, staying on the safe side I'm going the route of resuming into the TypeMonitor code.  Got it fixed up, and the bailout patch introduces only one new test failure which I will track down after checkin.

Comment 27

[Kannan Vijayan [:djvj]]

(In reply to Jan de Mooij [:jandem] from comment #25)
> Comment on attachment 707174 [details] [diff] [review]
> Baseline to Ion OSR, try 3
> 
> Review of attachment 707174 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nit: don't need this function if you pass BaselineFrame * to the stub (see
> other comment)
...
> 
> If you pass BaselineFrame * (masm.loadBaselineFramePtr(R0.scratchReg(),
> R0.scratchReg()), it would slightly simplify some (C++) code I think.

I agree, but I'd like to leave that for a delta patch after this one.  Whichever pointer I pass in, I'll need to compute the other one for certain purposes.  If we're passing in the BaselineFrame pointer, then the IonJSFrameLayout pointer needs to be calculated for the memcpy() of the retval+descriptor+callee+actualargs memory.

It's cleaner to do it the way you suggest, but it would best be done by landing a few cleanups in m-i first - namely: fixing the const attributions of various methods in IonJSFrameLayout, adding a "IonJSFrameLayout *BaselineFrame::ionJSFrame()" method to BaselineFrame.  And re-writing a bunch of the methods on BaselineFrame (e.g. callee(), numActualArgs(), etc.) in terms of the ionJSFrame method.

I'll file a bug for this.

Other comments addressed and pushed:
https://hg.mozilla.org/projects/ionmonkey/rev/13f62a92819a

Comment 28

[Kannan Vijayan [:djvj]]

Expand PC mappings patch: https://hg.mozilla.org/projects/ionmonkey/rev/72b12f35a967

Comment 29

[Kannan Vijayan [:djvj]]

(In reply to Jan de Mooij [:jandem] from comment #16)
> Comment on attachment 705947 [details] [diff] [review]
> Bailout from Ion to Baseline
>
> ::: js/src/ion/BaselineJIT.cpp
> @@ +734,5 @@
> > +        //      STACK_START_ADDR + IonJSFrameLayout::Size() + PREV_FRAME_SIZE
> > +        //                      - IonBaselineStubFrameLayout::reverseOffsetOfSavedFramePtr()
> > +        if (type == IonFrame_BaselineStub) {
> > +            size_t offset = IonJSFrameLayout::Size() + topFrame->prevFrameLocalSize() +
> > +                            IonBaselineStubFrameLayout::reverseOffsetOfSavedFramePtr();
> 
> Shouldn't this be offsetOfSavedFramePtr, since we have a pointer to the
> start of BaselineStub layout?
> 
> @@ +754,5 @@
> > +            return NULL;
> > +
> > +        // Otherwise, the frame preceding the rectifier is a BaselineStub frame.
> > +        size_t extraOffset = IonRectifierFrameLayout::Size() + priorFrame->prevFrameLocalSize() +
> > +                             IonBaselineStubFrameLayout::reverseOffsetOfSavedFramePtr();
> 
> Same here.

These offsets subtract from the layout pointer to arrive at the address of the saved frame pointer.  The stub frame pushes the stub pointer, and then the frame pointer, so it's lower in memory.  Using same convention as with BaselineFrame.

> @@ +925,5 @@
> > +        IonSpew(IonSpew_BaselineBailouts, "      Is function!");
> > +        IonSpew(IonSpew_BaselineBailouts, "      thisv=%016llx", *((uint64_t *) &thisv));
> > +
> > +        size_t thisvOffset = builder.framePushed() + IonJSFrameLayout::offsetOfThis();
> > +        *((Value *) builder.pointerAtStackOffset(thisvOffset)) = thisv;
> 
> We could add a setThisValue() to BaselineFrame, like thisValue()
> 
> To avoid exposing everything and having to add setters, we can also move
> this code to BaselineFrame::initFromBailout. It would match
> StackFrame::initFromBailout and may be a bit more readable. What do you
> think?
> 
> @@ +936,5 @@
> > +            Value arg = iter.read();
> > +            IonSpew(IonSpew_BaselineBailouts, "      arg %d = %016llx",
> > +                        (int) i, *((uint64_t *) &arg));
> > +            size_t argOffset = builder.framePushed() + IonJSFrameLayout::offsetOfActualArg(i);
> > +            *((Value *) builder.pointerAtStackOffset(argOffset)) = arg;
> 
> blFrame->formals()[i] = arg;


Both of these need to be done by calculating the offset size and then using pointerAtStackOffset on the builder.  This is because the calculated address may end up being inside the actual C stack instead of the heap-space currently being reconstructed.  In particular, the this value pointer, or arg value pointers, for the first reconstructed frame, will reach into the arguments and this value pushed by the caller, which will be on the C stack.


> @@ +1046,5 @@
> > +            // into registers.
> > +            frameSize -= sizeof(Value) * numUnsynced;
> > +            *(blFrame->addressOfFrameSize()) = frameSize;
> > +            IonSpew(IonSpew_BaselineBailouts, "      Adjusted framesize -= %d: %d",
> > +                            (int) (sizeof(Value) * numUnsynced), (int) frameSize);
> 
> Nit: I don't think you need the (int) casts, or else you can make them
> C++-style casts: int(frameSize).

These may be size_t, which is larger than int, and would not end up getting default-argument-casted into an int-sized value.  Changing them to C++-style casts.


> Nit: move the actual bailout code to BaselineBailouts.cpp (does not need its
> own header file, but the cpp code is pretty self-contained)

I'll do this before checking in.  Going to post fixed up bailout patch now.

Comment 30

[Kannan Vijayan [:djvj]]

Attachment: Bailout from Ion to Baseline, Try 2

Second stab.  This fixes a bunch of test failures that showed up with the previous patch, and addresses all comments except those noted above.

Comment 31

[Jan de Mooij [:jandem]]

Comment on attachment 707818 [details] [diff] [review]
Bailout from Ion to Baseline, Try 2

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

Great! r=me with nits addressed and cpp code moved into its own file.

::: js/src/ion/BaselineCompiler.cpp
@@ +508,5 @@
>  
>  bool
>  BaselineCompiler::emit_JSOP_POP()
>  {
> +    // Ion captures resumeAfter points after some pops, leave canResumeAfter_ true.

Nit: you can remove this comment now

::: js/src/ion/BaselineJIT.cpp
@@ +418,5 @@
>      JS_ASSERT(script->baseline == this);
>  
>      uint32_t pcOffset = pc - script->code;
> +    if (pcOffset == 0)
> +        return method_->raw() + prologueOffset_;

I think we should move this into the caller, and only resume at the prologue when we have no scope chain. There are two cases if pcOffset == 0:

(1) If the scope chain is not set, resume before the prologue
(2) Else, resume at the first op (to avoid creating a call object twice)

@@ +624,5 @@
> +        }
> +        return true;
> +    }
> +
> +    bool writeBaselineFrame(const BaselineFrame &blFrame, const char *info) {

Nit: this method is not used

@@ +671,5 @@
> +            break;
> +        }
> +    }
> +
> +    void setIncomingStack(void *incomingStack) {

Nit: method is not used

@@ +687,5 @@
> +    void setMonitorStub(ICStub *stub) {
> +        header_->monitorStub = stub;
> +    }
> +
> +    inline size_t dataSize() const {

Nit: ditto

@@ +1164,5 @@
> +    for (unsigned i = 0; i < actualArgc + 1; i++) {
> +        size_t argSlot = ((script->nfixed + exprStackSlots) - (actualArgc + 1) - 1) + i;
> +        if (!builder.writeValue(*blFrame->valueSlot(argSlot), "ArgVal"))
> +            return false;
> +        // FIXME: delete below

Nit: delete

::: js/src/ion/Ion.cpp
@@ +1825,5 @@
>  
> +        // If it's on the stack with an ion-compiled script, and it has a baseline script,
> +        // then keep the baseline script around (by marking it active), since bailouts from
> +        // the ion jitcode might need to re-enter into the baseline jitcode.
> +        if (invalidateAll && it.script()->hasBaselineScript())

We will probably also want to do this for inlined frames (create an InlineFrameIterator, loop and mark scripts as active).

::: js/src/ion/IonMacroAssembler.cpp
@@ +603,5 @@
> +        Register reg2 = regs.takeAny();
> +        Register reg3 = regs.takeAny();
> +
> +        // Copy data onto stack.
> +        Register copyCur = reg1;

Nit: reg1-3 are not used for anything else, so you can just do copyCur = regs.takeAny() etc

Comment 32

[Kannan Vijayan [:djvj]]

Bailout patch: https://hg.mozilla.org/projects/ionmonkey/rev/ed51ae24fee2

Comment 33

[Jan de Mooij [:jandem]]

Filed bug 843596 for OSR from the interpreter.