486699 - Support running the JIT late, after Verifier already has run

Status: RESOLVED FIXED

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

Description

[Edwin Smith]

this is a step on the way to better managing of code memory.

Comment 1

[Edwin Smith]

Attachment: renaming and refactoring only

renames:
AtomMethodProc -> GprMethodProc
DoubleMethodProc -> FprMethodProc
interp32 -> interpGPR
interpN -> interpFPR
impl32 -> implGPR
implN -> implFPR

refactoring:

* call interpGetOpcodeLabels() from WordCodeEmitter.cpp, remove ifdefs around constructor

* inline CallStackNode.exit() into the one place its called, clean up cruft.

Comment 2

[Steven Johnson]

Comment on attachment 370889 [details] [diff] [review]
renaming and refactoring only

some comments still refer to interpA/N, otherwise looks good

Comment 3

[Edwin Smith]

Comment on attachment 370889 [details] [diff] [review]
renaming and refactoring only

pushed http://hg.mozilla.org/tamarin-redux/rev/dfe3cb9aca6d

Comment 4

[Edwin Smith]

Attachment: always use a MethodInfo to wrap IMT stubs

Previously, an interface method dispatch stub was only referenced by a MethodEnv.  The VMCFG_METHODENV_IMPL32 config switch determined whether a MethodInfo stub was used, and if so, one stub is created per MethodEnv.

IMT stubs were tagged with a 3bit tag (BKIND_ITRAMP), which required Nanojit to always align them on 8-bit boundaries, possibly by inserting NOP instructions.

new behavior:
- Traits.m_imt[] will contain a MethodInfo* tagged with BKIND_ITRAMP, instead of a raw code pointer.  mi->implGPR points to the code.  
- nanojit no longer aligns code, since it doesn't need to.
- if VMCFG_METHODENV_IMPL32 == 0, dispatch is via env->method->implGPR like any other method
- if VMCFG_METHODENV_IMPL32 == 1, dispatch is via env->implGPR just as before.  The code pointer is obtained from method->implGPR.  We don't use delegateEnter() because it clobbers the invisible IID register. (comments added).

Comment 5

[Steven Johnson]

Comment on attachment 371245 [details] [diff] [review]
always use a MethodInfo to wrap IMT stubs

nit: is using +- to set/clr BKIND_ITRAMP worthwhile? (ie is the code generated that much better?) in this case we assert that everything is as we expect but it feels open for a cut-n-paste bug...

Comment 6

[Edwin Smith]

its better, by a couple instructions.  In this case, may not move the needle.  But better to make the code better in small increments than not at all, assuming low risk (thats my assumption).  Note that using + or | both depend on the tag already being zero.  Using - instead of & is slightly less robust, but if one is un-tagging something that isn't already tagged, the bug is more severe either way.

Comment 7

[Edwin Smith]

Comment on attachment 371245 [details] [diff] [review]
always use a MethodInfo to wrap IMT stubs

pushed 
http://hg.mozilla.org/tamarin-redux/rev/700e8228e0b2

Comment 8

[Edwin Smith]

Attachment: adds ResolverWriter to encapsulate verifier side effects

factors the bodies of OP_newfunction, OP_newclass, OP_newactivation into a ResolverWriter, and adds new functionality to ensure the side effect only happens once.

When an OP_newfunction/newclass is seen a second time, capture the scope chain again and compare it to the originally captured one.  Verify fails if the two scope chains are not type compatible.  (compared to the existing behavior, where the side effects from second & subsequent occurances would be ignored).

If the bytecode actually contains more than one OP_newfunction or OP_newclass call site, this means the first one verified determines the type/shape of the captured scope chains for all others.  This is not ideal since the first to be verified is determined by execution order.  On the other hand, this change puts teeth on what could already happen.

Comment 9

[Tom Harwood]

Comment on attachment 375018 [details] [diff] [review]
adds ResolverWriter to encapsulate verifier side effects

Looks good.  Why use a new CodeWriter, though?  I'm not seeing any logic that wouldn't have fit into the big honkin' switch.

Comment 10

[Edwin Smith]

Verifier is on a refactoring trajectory where code is moving out of the switch into a set of CodeWriter subclasses, mainly for the purpose of separation of concerns.

I haven't tried this yet, but I am also thinking that if we're running the verifier multiple times, the 2..N invocations could actually simply not use ResolverWriter; unplug it from the coder chain except for un-verifierified methods.

Comment 11

[Tom Harwood]

> 2..N [Verifier] invocations

Good point.  A lot of the Verifier doesn't need to be re-run.

Comment 12

[Rick Reitmaier]

Just a wild thought...wonder if patterning the verifier similar to the interpreter, would provide any benefits.

Comment 13

[Edwin Smith]

I have no idea what you mean

Comment 14

[Rick Reitmaier]

I was thinking about using the INSTR() macros to encapsulate the abc bytecode logic; removing the switch and gaining direct-threading support essentially for free.  

But in looking further at the verifier code, this really doesn't make much sense as the dispatch cost per opcode is so small relative to what's being done.

Comment 15

[Edwin Smith]

direct threading in the verifier would not make sense, we'd have to translate to direct-threaded code, just to verify.  cart-before-horse.  not to mention, switch dispatch overhead in verifier is nil compared to other work, as you pointed out.

also, as chunks of code in verifier case blocks become more similar due to CodeWriter refactoring, more and more cases will become identical and can be collapsed.  (ie case OP_foo: case OP_bar: ...)  the INSTR() style requires a distinct code block for every opcode, no collapsing allowed.