528111 - Runtime support for Ahead-of-Time (AOT) Compilation

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Stan Switzer]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5
Build Identifier: Tamarin Redux, 11 Nov 09

This patch updates the AVM runtime to support Ahead-of-Time compilation.  This involves:

 - Locating precompiled method code
 - Capturing traits so that they can be referred to by precompiled code.
 - Activation Traits for precompiled methods
 - Traits initialization that would normally be done by the verifier
 - Debugger support

These changes are separate from the fixed-slot-offset changes (which is a separate patch).
 

Reproducible: Always

Steps to Reproduce:
New feature

Comment 1

[Stan Switzer]

Attachment: Proposed AOT support patch

Corresponds largely to Perforce changelist 601503 minus the slots changes.

Comment 2

[Steven Johnson]

Comment on attachment 411870 [details] [diff] [review]
Proposed AOT support patch

No red flags, mostly just code style comments:

-- all new code should use C99 integer types where possible, avoiding  
"int"/"long"/"unsigned" unless absolutely required (typically, only for printf/scanf).

-- code style nit: preferred style in Tamarin is to avoid Hungarian  
warts ("nStateTraits" -> "stateTraitsCount" or similar)

-- AbcParser references some undefined vars, presumably constants declared by the AOT compiler… eg "nAOTInfos". New constants should ideally be declared with a "k" prefix or in all caps, eg "kAOTInfoCount" or "AOT_INFO_COUNT".

-- AbcParser::canParse: do we really want to use 0 as the "magic"  
value for AOT? 

-- if you want a function inlined, use REALLY_INLINE rather than plain  
"inline". (sad but necessary) (and yes, there are plenty of existing  
places this is violated in existing code, we're slowly migrating them  
all)

-- code style nit: preferred style in Tamarin is "if (cond)", not "if 
(cond)"

-- code style nit: preferred style in Tamarin is to use NULL for null  
pointers (vs plain "0")

-- code style nit: if an argument is being passed by a pointer-that-can't-ever-be-null, use a reference rather than a pointer (eg NativeInitializer::getCompiledInfo)… yes, even for "out" parameters.

Comment 3

[Edwin Smith]

Comment on attachment 411870 [details] [diff] [review]
Proposed AOT support patch

MathUtils.cpp: intentional deletion of code?

MethodInfo.h: when code is rebased, inline method bodies should all be in MethodInfo-inlines.h

Remainder looks safe (everything looks carefully ifdef'd) but I haven't reviewed the aot-code carefully.  It really needs a bit of documentation to explain the larger context.

Comment 4

[Steven Johnson]

pushed (with minor tweaks) as changeset:   3136:7b6e3a3be69d

Comment 5

[Stan Switzer]

Responding to comments...

nAOTInfos -> kAOTInfoCount, OK.  AHould be defined in AOTCompiled.h which I accidentally dropped from the patch (dropped all additions... driver error).

zero as a magic number?  Well, since the ABC bodies are built into the binary, there can never be version skew. It corresponds to no loadable version of the ABC format.  (Consists only of the runtime metadata and no code bodies.)  Zero makes a certain sense to me, but YMMD.

I think in some cases we don't care if it really inlines and are happy to give the compiler the choice.  But if a function (as opposed to a method) is defined in a header, it needs to say inline or else the linker will see multiple definitions.

"if (cond)", not "if (cond)"  ? Sorry, confused.

"yes, even for "out" parameters."  Surely you jest!  Well, when in Rome, I guess.

Shall we put these on a list to revise next go-round?

Also, arguably the private version of ScopeChain::create I introduced should be public and the other two versions that call it should be removed.  I did it this way to minimize the number of changes that were not _directly_ AOT-related.  Making that change touches half a dozen files and so I decided better of it, but it would address the FIXME referring to the use of FrameState in this function.


MathUtils.cpp:  Pops, sorry, that was unrelated.  Some freelance cleanup.  I gasped in surprise at "#define NAN acosh(0)" and then discovered it was never used.  So I deleted it.

Comment 6

[Chris Peyer]

Engineering work item.  Marking as verified.

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

(Felix: adding reference to pushed changeset long after it was pushed.)

changeset:   3145:7b6e3a3be69d
parent:      3135:f454ce7466f4
user:        Steven Johnson <[e-mail]>
date:        Mon Nov 16 13:56:02 2009 -0800
summary:     add runtime support for Ahead-of-Time compilation. (r=stejohns, sr=edwsmith, bug=528111)

http://hg.mozilla.org/tamarin-redux/rev/7b6e3a3be69d