570521 - With minor changes, interpGPR() and interpFPR() become jit-only helpers

Status: RESOLVED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P3)

Description

[Edwin Smith]

A medium term goal is to isolate all the code using Gpr/FprMethodProc signatures to the jit-specific parts of ExecMgr, to enable the JIT to customize the jit->jit calling conventions.

the stubs interpGPR() and interpFPR() are currently invoked in only two circumstances

1. JIT code calling early-bound interpreted code
2. Function.apply/call invoking interpreted code for the first time (via verifyInvoke)

This bug aims to eliminate case 2.  It exists because BaseExecMgr::apply() (and call()) use a heuristic to detect whether they need the full (expensive) argument unboxing path, which can alloca() twice, or the optimized/specialized path that unpacks and coerces in one pass.  The logic is:

    if (isInterpreted(env))
       /* slow case */
    else
       /* fast case */

instead, propose adding a flag to request the fast case, and that flag is only set once verification is complete and we know the execution mechanism, thus forcing the first call always to take the slow path, and for subsequent interpreter calls to continue taking the slow path (as before).

    if (/* apply_fast_path flag set */)
      /* fast path */
    else
      /* slow path */

In the slow path, invoke_interp() ultimately gets called instead of interpGPR/FPR.

Comment 1

[Edwin Smith]

Attachment: Add APPLY_FASTPATH flag, and use it to avoid calling interpGPR/FPR via Function.call/apply

This patch is a saved work in progress.  It moves the interpGPR/FPR stubs to exec-jit.h.  The only way they can be called in interpreter-only configurations is on the first call, when isInterpreted() returns false on a method that hasn't been verified yet.

Notes & TODO list:

Interpreter mode: interpGPR/FPR are only called in one edge case
by Function.call/apply, which is easily fixed.  Then the stubs are only
need by jit->interp transitions.

MethodInfoProcHolder._implGPR/FPR is then only used by jit calls,
and by trampolines in C++ that are invoked by jit calls, e.g.
verifyEnterGPR().  Native functions called by C++ code go through
an invoker that can use implGPR or can use _native.thunker.

verifyEnterGPR/FPR called for jit or native methods.  This is overkill for
native methods if we can guarantee they're resolved before being called.

todo
* (done) move interpGPR/FPR to exec-jit.cpp
* add debugEnter/Exit invoker wrappers (variants of invoke_generic).
* hide NativeInitializer and NativeInfo?
* hide exception handling logic

Comment 2

[Edwin Smith]

Attachment: (v2) Only use interpFPR/GPR for jit->interp transitions

Asking for feedback to make sure this doesn't break old assumptions about how Function.apply/call invoke the interpreter.

1. Before this patch, when Function.apply/call invoked the interpreter, they unpack the arguments then call coerceEnter(), which calls interpBoxed, which alloca's the interpreter variable frame and copies atoms into it.  So, two alloca calls along this path.

2. Before this patch, if the very first call to an interpreted method was via Function.apply(), then isInterpreted() would be false (not yet verified) and so we'd use alloca, then unboxCoerceArgs to unpack the call/apply argumetns into a *native* arguments array, then run interpBoxed.  still, two alloca calls, but unnecessary unboxing.  this is a bug, although very minor.

After this patch, in scenario 1, the same thing happens, and scenario 2 is just like scenario 1:  we still do two alloca calls but we don't do any unboxing.

thus: minor bug fixed.  More importantly, interpGPR/FPR are now only ever invoked by jit code calling the interpreter.  Future work will modify the JIT ABI, and remove these stubs and use jit-compiled ones instead.

Comment 3

[Edwin Smith]

(In reply to comment #2)
> 2. Before this patch, if the very first call to an interpreted method was via
> Function.apply(), then isInterpreted() would be false (not yet verified) and so
> we'd use alloca, then unboxCoerceArgs to unpack the call/apply argumetns into a
> *native* arguments array, then run interpBoxed.  still, two alloca calls, but
> unnecessary unboxing.  this is a bug, although very minor.

(grr, review before submit).  This should read:

 2. Before this patch, if the very first call to an interpreted method was via
Function.apply(), then isInterpreted() would be false (not yet verified) and so
we'd use alloca, then unboxCoerceArgs to unpack the call/apply argumetns into a
*native* arguments array, then run interpGPR or interpFPR, which re-box the native
arguments in-place, then call interpBoxed.  still, two alloca calls, but
unnecessary unboxing and re-boxing.  this is a bug, although very minor.

Comment 4

[Lars T Hansen]

Comment on attachment 467020 [details] [diff] [review]
(v2) Only use interpFPR/GPR for jit->interp transitions

Can't think of anything unreasonable about this.

(May want to document _apply_fastpath in MethodInfo.h.)

Comment 5

[Edwin Smith]

Attachment: (v3) Only use interpFPR/GPR for jit->interp transitions

Changes since v2:
 * rebased
 * consolated all setInterp() code in one function.  The cure for
   #ifdefs was worse than the disease, and injected a NPE bug.

Summary:

This patch fixes a benign bugin Function.apply/call; the bug was
allowing the interpreter to be invoked via a fast path that was
intended to only be used by jit/native methods.  With the bug fixed,
the only calls to interpFPR/GPR are from JIT code.  Any call to the
interpreter from C++ goes through coerceEnter and one of the invoke
methods.

So, the patch moves the interpGPR and interpFPR stubs into exec-jit.cpp
and appropriately #ifdefs the code that sets up pointers to these stubs.

Comment 6

[Edwin Smith]

http://hg.mozilla.org/tamarin-redux/rev/ee1b01936659