816053 - BaselineCompiler: Add IC stub for JIT -> JIT calls

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Works on x86, x64 and ARM. Some notes:

* Stubs are added for every callee, so that IonMonkey knows exactly which functions we have seen and can potentially inline. When we reach the stub limit, we stop adding stubs. Later we can just replace all stubs with a single stub which handles all scripted functions.

* The call stub first tries to call script->ion, then script->baseline. This means when the callee is Ion-compiled we will automatically start calling the optimized code. Since the calling convention is the same this is pretty straight-forward.

In the future, we can add a raw code pointer to JSScript, which is either script->baseline->method()->raw() or script->ion->method()->raw(). This will avoid the test and a bunch of loads, but it's probably better to do this later since it involves some IonMonkey changes.

* Includes the stub frame type we discussed. Does not yet mark the stub pointer stored in it, it's not needed atm since we don't remove stubs yet and all stubs are marked. I can file a follow-up bug for it since this patch is already getting pretty large.

Comment 2

[Kannan Vijayan [:djvj]]

Comment on attachment 686068 [details] [diff] [review]
Patch

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

Nice work!  Everything fits together really nicely.

::: js/src/ion/BaselineIC.cpp
@@ +904,5 @@
> +        // Script does not have a valid IonScript, see if it has a BaselineScript.
> +        masm.loadPtr(Address(callee, offsetof(JSScript, baseline)), callee);
> +        masm.branchPtr(Assembler::BelowOrEqual, callee, ImmWord(BASELINE_DISABLED_SCRIPT), &failure);
> +        masm.loadPtr(Address(callee, BaselineScript::offsetOfMethod()), callee);
> +    }

This could use masm.loadBaselineOrIonCode(callee)

When this stub is created, we already check if the script has either Ion or Baseline jitcode.  If Ion jitcode existed, then we can assume that Baseline jitcode will exist since baseline will always be compiled before we get to Ion.  If baseline jitcode existed, we can assume that it will continue to exist since we will never invalidate baseline jitcode.

::: js/src/ion/IonFrames-inl.h
@@ +100,5 @@
>      if (returnAddrOut)
>          *returnAddrOut = (void *) iter.returnAddressToFp();
>  
> +    if (iter.isBaselineStub())
> +        ++iter;

Would be good to JS_ASSERT(iter.isBaselineJS()) after ++iter (within the conditional), since all BaselineStub frames should sit immediately on top of a BaselineJS frame.

::: js/src/ion/arm/BaselineHelpers-arm.h
@@ +99,5 @@
> +    // Compute stub frame size. We have to add two pointers: the stub reg and previous
> +    // frame pointer pushed by EmitEnterStubFrame.
> +    masm.mov(BaselineFrameReg, reg);
> +    masm.ma_add(Imm32(sizeof(void *) * 2), reg);
> +    masm.ma_sub(BaselineStackReg, reg);

Note: So long as we're using the ma_* methods here, we may be able use the 3-operand variants to save some instructions - e.g.:  ma_add(BaselineFrameReg, Imm32(sizeof(void *) * 2), reg)

There are other places where some of the arm codegen I wrote has the same problem, so this may be better fixed by a general "optimize ARM BaselineHelper functions" bug.

@@ +110,5 @@
> +{
> +    EmitCreateStubFrameDescriptor(masm, r0);
> +    masm.push(r0);
> +    masm.call(target);
> +}

Nit: Can we add a TODO here to remind us to go back and change r0 to use the configurable scratch register on ARM after that change has been made in the mainline codebase and merged back?

::: js/src/vm/Stack.cpp
@@ +1603,1 @@
>          return ionInlineFrames_.isFunctionFrame();

Nit: this whould be better in if/else form as opposed to shortcut-return form - since we're logically making a choice between the two options.

@@ +1681,3 @@
>              return ionInlineFrames_.callee();
>          JS_ASSERT(ionFrames_.isNative());
>          return ionFrames_.callee();

Nit: better expressed as "if/else if/else" form as opposed to using shortcuting returns.  Reflects intent better.

Comment 3

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/projects/ionmonkey/rev/50e2cec07ebd

(In reply to Kannan Vijayan [:djvj] from comment #2)
> This could use masm.loadBaselineOrIonCode(callee)

Done. Looking at JM and TI data, we will probably want to allow GC'ing baseline code that's not active on the stack. We can deal with this later though, and in that case we will probably also want to reset our IC's anyway. Calling loadBaselineOrIonCode nicely simplifies the code for now.

> Would be good to JS_ASSERT(iter.isBaselineJS()) after ++iter [...]

Done.

> 
> There are other places where some of the arm codegen I wrote has the same
> problem, so this may be better fixed by a general "optimize ARM
> BaselineHelper functions" bug.

Agreed, will file a bug.

> Nit: Can we add a TODO here to remind us to go back and change r0 to use the
> configurable scratch register on ARM after that change has been made in the
> mainline codebase and merged back?

Not sure, the danger is that we call a MacroAssembler method that uses the scratch, so it seems best to leave this register alone outside the assembler. What do you think?

> Nit: better expressed as "if/else if/else" form as opposed to using
> shortcuting returns.

The style guide has a "no else after return" rule..