529833 - We can jit-compile coerceUnboxEnter

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Edwin Smith]

When this function is jit-compiled, all boxing/unboxing, coercing, and argument count checking can be done with known values and types, and unrolled.

when closure or event handler invocation is common, this approach is a win even over just specializing the subcases in bug 529832

Comment 1

[Edwin Smith]

Attachment: encapsulate isInterpreted()

Comment 2

[Edwin Smith]

Comment on attachment 413599 [details] [diff] [review]
encapsulate isInterpreted()

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

Comment 3

[Edwin Smith]

Attachment: MethodEnv::isInterpreted delegates to MethodInfo::isInterpreted

This also avoids the problem that MethodEnv->implGPR sometimes points to delegateEnter and gives a false negative.  The extra load is a tiny overhead but with upcoming changes to coerceEnter() call paths, this function will move off the critical path.

Comment 4

[Edwin Smith]

Attachment: factor argc error path out of inlined startCoerce()

It's a slow path and doesn't need inlining, and soon we will need to call it from jit code directly.

Comment 5

[Edwin Smith]

Comment on attachment 415392 [details] [diff] [review]
factor argc error path out of inlined startCoerce()

http://hg.mozilla.org/tamarin-redux/rev/40414973b229

Comment 6

[Edwin Smith]

Comment on attachment 415389 [details] [diff] [review]
MethodEnv::isInterpreted delegates to MethodInfo::isInterpreted

http://hg.mozilla.org/tamarin-redux/rev/4ce206f11fe9

Comment 7

[Edwin Smith]

Attachment: Refactor MethodInfoProcHolder and add setNativeImpl to encapsulate access to function pointers

MethodInfoProcHolder is always constructed in an initial state pointing to verifyEnterGPR, so we don't need the parameter; this is better encapsulation.

setNativeImpl() will get more complex soon, encapsulate access to MethodInfo::_implGPR/FPR function pointer fields.

Comment 8

[Edwin Smith]

Comment on attachment 415406 [details] [diff] [review]
Refactor MethodInfoProcHolder and add setNativeImpl to encapsulate access to function pointers

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

Comment 9

[Edwin Smith]

Attachment: use 0-arg form of coerceEnter() when possible

We have an overloaded form of coerceEnter for the 0 arg case; use it when possible.

Comment 10

[Edwin Smith]

Attachment: Adds function pointer for coerceEnter calls, plus specialized overloads for the no-coerce case

Posting now for early comments, no review required yet.

four specialized cases supported (this is before the jit patch):

1. interp            general case for calling interpreter
2. interp_nocoerce   specialized for no typed args (any arg count, esp. 0)
3. generic           general case for jit or native methods
4. generic_nocoerce  specialized for no typed args

in testing, #1 and #4 are hardly ever called.  Once the jit patch lands, we probably don't need #4 at all.  #2 is common for global and static initializers.  #4 is always used for getters and any other 0-arg function.

Comment 11

[Edwin Smith]

Comment on attachment 415469 [details] [diff] [review]
use 0-arg form of coerceEnter() when possible

http://hg.mozilla.org/users/edwsmith_adobe.com/enter-jit/rev/04ed1f987598

note: this patch further inhibited tail calls, since the 0-arg form of coerceEnter cannot tail-call interpBoxed(), while the 3-arg form can.

that will be fixed in the next patch.

Comment 12

[Edwin Smith]

Attachment: Adds function pointer for coerceEnter calls (v2)

First, this patch introduces a function pointer used for invoking any function without knowing its signature.  (used in all late bound situations, as well as for closure invocation).  "invoker" handlers are specialized in the context of the called method, where the expected arg count and argument types are constant.

For interpreted functions, this patch supports two specialized coerceEnter handlers; a generic one, and one which is used when no arguments need to be coerced.  This includes the 0-arg case that previously was in MethodEnv::coerceEnter(Atom thisArg).  Both tail-call to interpBoxed().

interpBoxed() no longer takes a MethodSignature parameter, which prevented tail-calls from working on x86, or any other cpu with less than 4 argument registers (are there any?)

For native and jit'd functions, coerceEnter_generic() is used, which corresponds to the old coerceUnboxEnter() function.  An upcoming patch will add a jit-compiler for specialized handlers for these two cases as well.

FunctionObject::as3_call was simplified, the argc == 0 case was moved into coerceEnter(thisArg, argc, args*) (ie the one overloaded for Function.call style invocation).

Finally, MethodEnv::coerceEnter(Atom) and (argc, Atom*) are now simply one-liner inline functions that invoke via the new function pointer.

Future: we could generate C++ handlers for native methods in nativegen.py, instead of using the JIT or the generic invoker.

Comment 13

[Edwin Smith]

In interpreter-only configurations, we will never invoke any function via the "native" ABI (i.e. via the GprMethodProc or FprMethodProc signatures).  So, we could eliminate the MethodEnv and MethodInfo._implGPR/FPR pointers and compile the VM with only generic late bound thunks.

Comment 14

[Edwin Smith]

Attachment: DEBUG code to clobber coerceEnter args after the call

Protecting the callee's right to overwrite incoming args.

Comment 15

[Lars T Hansen]

Comment on attachment 415859 [details] [diff] [review]
DEBUG code to clobber coerceEnter args after the call

Nifty!

Comment 16

[Lars T Hansen]

Comment on attachment 415786 [details] [diff] [review]
Adds function pointer for coerceEnter calls (v2)

Skimmed it but looks good.

Comment 17

[Edwin Smith]

Comment on attachment 415786 [details] [diff] [review]
Adds function pointer for coerceEnter calls (v2)

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

Comment 18

[Steven Johnson]

(In reply to comment #14)
> Created an attachment (id=415859) [details]
> DEBUG code to clobber coerceEnter args after the call

instead of memset to 0, perhaps memset to a more obviously poisoned value (eg 0xfb)?

Comment 19

[Edwin Smith]

Attachment: Moves helpers from CodegenLIR to LirHelper, adds a bunch more.

This doesn't change any functionality, just moves code around and adds several new helpers that will be used by both CodegenLIR and the new invoker compiler.

Comment 20

[Rick Reitmaier]

Comment on attachment 416125 [details] [diff] [review]
Moves helpers from CodegenLIR to LirHelper, adds a bunch more.

Rubber stamping, let me know if you'd like a more thorough review.

Comment 21

[Edwin Smith]

as long as you dont have concerns about the direction of this refactoring arc, then we're good.  (a helper per instruction greatly cleans up code).

Comment 22

[Rick Reitmaier]

If it cleans up code, r++

Comment 23

[Edwin Smith]

Comment on attachment 416125 [details] [diff] [review]
Moves helpers from CodegenLIR to LirHelper, adds a bunch more.

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

Comment 24

[Edwin Smith]

testing on Flex apps shows the extra code growth from JIT-compiling these methods is typically 5% and as high as 12%, using the policy that the first call via coerceEnter triggers compilation if the underlying method is JIT compiled or native.

All other app frameworks tested (HaXe, Alchemy, Papervision) had smaller growth than Flex.

Comment 25

[Edwin Smith]

Attachment: use new helper functions throughout CodegenLIR

More code cleanup; nothing functional should have changed.

Comment 26

[Rick Reitmaier]

Comment on attachment 416616 [details] [diff] [review]
use new helper functions throughout CodegenLIR

Now that the storeIns()'s have been separated, should we put asserts inside stq/sti/stp to capture bad args?

Also, I wonder if optional_label is really optional?

Comment 27

[Edwin Smith]

Attachment: mark ms volatile to make it setjmp/longjmp safe.

Reloading ms in the catch block but that doesn't suppress the warning.  Since access is infrequent, and several other longlived variables are marked volatile, this patch makes ms volatile and cures the warning.  safe?

Comment 28

[Edwin Smith]

Comment on attachment 416616 [details] [diff] [review]
use new helper functions throughout CodegenLIR

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

Comment 29

[Edwin Smith]

Comment on attachment 416789 [details] [diff] [review]
mark ms volatile to make it setjmp/longjmp safe.

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

Comment 30

[Edwin Smith]

Attachment: move more shared stuff up to LirHelper

Comment 31

[Rick Reitmaier]

Comment on attachment 416965 [details] [diff] [review]
move more shared stuff up to LirHelper

minor nit: duplicate keyword 'protected' near LirHelper decl.

Comment 32

[Edwin Smith]

Comment on attachment 416965 [details] [diff] [review]
move more shared stuff up to LirHelper

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

Comment 33

[Edwin Smith]

Attachment: jit stubs for coerceEnter(), for jit methods and native methods (not interpreted methods)

Adds JIT copiler for coerceEnter stubs.  These are stubs that unroll and specialize MethodEnv::coerceEnter_generic for the method being called.  In the context of the called method, the expected argument count, argument types, and even the address of the jit'd (or native) code is known, allowing lots of specialization.

On 64-bit cpu's and in most cases on 32-bit cpu's, the arguments are unboxed and coerced in-place.  However, on a 32-bit cpu, if at least one declared argument is a double, then the arguments must be copied.  If, in that case, the method wants rest args or arguments (varargs), then we must punt because we cannot currently jit code that does alloca().   (future: we could actually jit code that uses gc-alloc for the args, by inlining VMPI_alloca()).  Alternatively we could modify the ABI so that all Atom args are 8-aligned (ouch).

Comment 34

[Lars T Hansen]

Comment on attachment 417112 [details] [diff] [review]
jit stubs for coerceEnter(), for jit methods and native methods (not interpreted methods)

Nit: use REALLY_INLINE.

Comment 35

[Edwin Smith]

(In reply to comment #34)
> (From update of attachment 417112 [details] [diff] [review])
> Nit: use REALLY_INLINE.

good point, pushed as a separate patch. http://hg.mozilla.org/tamarin-redux/rev/149eff886640

Comment 36

[Edwin Smith]

Attachment: (v2) jit stubs for coerceEnter(), for jit methods and native methods (not interpreted methods)

Updated and commented.

Modified jit policy (for the coerceEnter stub only) to compile on second call; this reduces code size and compile time significantly for some Flex tests with no adverse performance effects.  (Flex apparently uses a high number of closures for one-time initialization tasks).

Comment 37

[Rick Reitmaier]

Comment on attachment 417474 [details] [diff] [review]
(v2) jit stubs for coerceEnter(), for jit methods and native methods (not interpreted methods)

[1] minor nit, names of invoker1 and invoker2 should be swapped for clarity
[2] outside of scope but the code required to create a 'compiler' ... new frag, lirbuf, lirwriter , etc is awkward would be nice to have something simpler, like : Complier comp(codeMgr, ABI_CDECL);   
[3] for optional args; was trying to think of another way to code it up (e.g. jmp indirect offset), as the multi-way branch seems inelegant.  But in practice i doubt this will be an issue.
[4] wonder if its worth exploring a faster intToAtom, uintToAtom.  E.g. jit a box/unbox  compare and jump to slow path if it fails.
[5] nice!

Comment 38

[Edwin Smith]

(In reply to comment #37)
> (From update of attachment 417474 [details] [diff] [review])
> [1] minor nit, names of invoker1 and invoker2 should be swapped for clarity
> [2] outside of scope but the code required to create a 'compiler' ... new frag,
> lirbuf, lirwriter , etc is awkward would be nice to have something simpler,
> like : Complier comp(codeMgr, ABI_CDECL); 

agreed, would be nice, soon

> [3] for optional args; was trying to think of another way to code it up (e.g.
> jmp indirect offset), as the multi-way branch seems inelegant.  But in practice
> i doubt this will be an issue.

I spent time prototyping a solution using switch(argc), roughly:

   // 2 required, 2 optional
   switch(argc) {
   case 4:
      /* downcast + unbox arg 4 */
   case 3:
      /* downcast + unbox arg 3 */
   case 2:
      break;
   default:
      /* throw arg count error */
   }
   /* downcast + unbox arg 2 */
   /* downcast + unbox arg 1 */

only to realize it breaks visible left-to-right evaluation order semantics.  (say the arg types are Number and the values are Object and we call valueOf() on each one).

so, the left-to-right solution is a series of short-circuit branches:

   /* downcast + unbox arg 1 */
   /* downcast + unbox arg 2 */
   if (argc < 3) goto done
      /* downcast + coerce arg 3 */
   if (argc < 4) goto done
      /* downcast + coerce arg 4 */
   done:
      /* do the call */

future note:  this all assumes we're going to invoke the underlying
method, which also has a series of branches for its own (typed) optional
arg processing, to select between the incoming value and the default value.
bug 512686 has more ideas on streamlining calls with optional args.

> [4] wonder if its worth exploring a faster intToAtom, uintToAtom.  E.g. jit a
> box/unbox  compare and jump to slow path if it fails.

Yes, all the cases for downcasting can benefit from inlining the no-convert/no-error path.  also, the subtypeof() test used in object downcasting can be heavily specialized.  will create followup bug.

> [5] nice!

thanks!

Comment 39

[Edwin Smith]

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