[PATCH] New JIT APIs (ORC)

[Lang Hames]

Hi Eric,

> Any hope of the "use orc under the covers patch" so that the existing JIT tests can be repurposed?

We could add an lli -use-orc line to pretty much every MCJIT regression test if we want. I don't think the MCJIT tests take long to run, so that's probably not too much overhead? 

> Few questions:
> You have a directory called Orc and files/libraries called OrcJIT. Probably move the directory to OrcJIT or change the other stuff to just be Orc?

Got a preference? I've leaned towards calling everything just Orc except for the OrcJIT.h header and OrcJITCtor, for symmetry with MCJIT. I'm happy to rename them to just "Orc" too though.

> Timing on the module splitting? Just curious what the overhead here is looking like.

You mean compile time overhead? I haven't tested that yet, but I expect it to be negligible compared to the overhead of the rest of CodeGen. Also, only people who want lazy compilation will pay for it. The MCJITReplacement class doesn't use it so there's no overhead for existing clients.

It's not directly related to your question, but I thought it was a neat statistic (and it provides a loose bound on this): Using lazydemo built with Debug+Asserts to lazily execute 401.bzip2 took 27s on my machine, vs 20s for a statically compiled -O2 binary. Given that that includes the full IR optimization/codegen/JIT overhead, and precludes any serious optimization of the program, that's a pretty good.

> Need to have all of the headers in include?

All the headers in include are intended for outside clients to be able to access directly. The lazydemo uses these to build its own custom JIT stack.

> Some function names are in StudlyCaps, others in camelCase?

I tried to stick to camelCase for actual functions and StudlyCaps for member variables, even where the latter are std::functions. I'm not sure we have an explicit convention for std::function members? (Also, I may have mixed them up in places - I'll try to check for that).

> C API - Did we want to define a new one here?

Not yet - this stuff is still very experimental. We may want to expose the UseOrcJIT flag temporarily at some point so that C API clients can test it, but even then it'd be nice if that didn't become permanent API.

> lookupSymbolAddressIn - some docs on why we'd need to do this in a module set would be good. :)

Sure. I'll try to add some more docs. The short version is that it's necessary to scope lookup in some places to make sure you resolve to the right symbol (e.g. if there are multiple "static void foo() {...}" functions, you want to make sure references to them resolve correctly).

> explode - Since it doesn't modify the existing module we'd have duplicate copies of IR functions here? i.e. we've indirected foo and and then exploded foo, but bar was never indirected and we might want to JIT bar at some point, but then we've got foo in the module as well?

Explode extracts all functions. I'm not a fan of this, but the way it works right now is that for each function where the ShouldExtract predicate returns true, we extract that function into its own Module, and for all functions where ShouldExtract returns false, we copy all of them together into a single new Module.

The explode function was written hastily to get the demo working. I think a better (and more general) way to think about carving up Modules is to have a map from Functions to new Modules. The biggest benefit is that you can then colocate functions that you want to be compiled together anyway. E.g.:

void foo(void) { bar(); }
void bar(void) { if (rand() % 16384 ==  0) baz(); }
void baz(void) { /* ... */ }

In this case, you'd want to map foo and bar to the same module, since any time you compile foo you know you're going to have to compile bar a moment later anyway, but you'd want to map baz to a different module because it gets called rarely.

> Handling compilation on another thread - any ideas for making that part of this and thread safe? :)

I don't have any plans for this, but I'd *love* to see it. That sounds cool.

> Otherwise I think you should feel free to commit and handle most of this stuff after - use your discretion.

Cheers. It'll be a mix, but I'll try to get the egregious stuff (e.g. MSVC and coding standards breaking syntax) fixed before I commit.

> Patch commentary below:

I agreed with all your commentary and I'm happy to make the suggested alterations. I've only responded below where it seemed particularly noteworthy (mostly on actual bugs or code changes, rather than comments or cosmetic stuff).

> 
> +    auto DylibLookup = std::make_shared<CODScopedLookup>(B);
> +    ModuleSetHandleT H =
> +        ModuleSetInfos.insert(ModuleSetInfos.end(), ModuleSetInfo(DylibLookup));
> 
> Should ModuleSetInfo own the lookup? I don't see a reason why it couldn't and then everything else have a reference, but I might have missed something.

I think you're right. This code was redesigned fairly recently and I think the rationale for shared_ptr is gone. I'll test that out and assuming I haven't missed anything I'll change it so that ModuleSetInfo owns the lookup.

> +      // Insert callback asm code into the first module.
> +      insertX86CallbackAsm(*ExplodedModules[0],
> +                           *MSI.JITResolveCallbackHandlers.back());
> 
> Add a fixme please? :) You know why.

Why? And what's this '-arch' thing people are always going on about?

I'm actually going to replace this with a std::function so that people can start writing their own callback injectors if they want to play with Orc.

> +      return {std::move(*Obj), std::move(ObjBuffer)};
> 
> Apparently this doesn't work in MSVC :)

No it doesn't. Thanks for the catch.

> +///   This class provides name mangling that can outlive a Module (and its
> +/// DataLayout).
> +class PersistentMangler {
> 
> FWIW it might be nice to have the mangler split out from DataLayout in some way some day.

Yeah - that would be great. Having to do this was unfortunate.

> +      if (EmitState)
> 
> Should check against an enum value yes?

Yep. Thanks!

> +    // Build the MangledNames map. Bails out early (with MangledNames set to
> +    // nullptr) if the given SearchName is found while building the map.
> +    void buildMangledNames(StringRef SearchName,
> 
> This seems odd, can you explain it for me? I'm probably missing something simple but I'm having an issue figuring out the early exit bit and how this works in general.

It's pretty horrible. Being a JIT library, rather than a JIT, Orc doesn't enforce a particular mangling scheme. Instead, it uses the target's. All lookups are done on mangled names. On ELF this is a no-op, but on MachO it means prepending an "_" everywhere. So, say you have a Module with a function "foo" in it. If you want to get the address for that function on Darwin you need to look up "_foo". (This keeps the JIT's symbol lookup consistent with RuntimeDyld's, since references in the ObjectFile will also be to "_foo"). In the LazyEmitLayer if you get a lookup of "_foo", you need to go through any IR you're holding and figure out whether any of your symbols mangle to "_foo". To avoid mangling redundantly, the first time we look in an IR module we want to build a map (buildMangledNames) to use on subsequent searches. In the common case however, you'll find a symbol on the first search, so we also don't want to build the whole map if we can avoid it. That's the reason for the early exit.

This whole mess could be avoided if we had an assembly demangler. Perhaps that's a sane thing to add?

> +  template <typename... CtorArgTs>
> 
> Do we get template varargs yet?

I don't think so, and no clients need it yet. I'll back that out.

> +  uint64_t getSymbolAddressInLogicalDylib(const std::string &Name) override {
> 
> Comment? In particular for the term "logical" :)

Perhaps "Pseudo" would be a better prefix? Basically "look up this symbol in the context of a bunch of modules as if they had been linked together as a dylib". In particular this is intended to expose global hidden symbols where getSymbolAddress should not.

> +                     HandleFunctionFtor HandleFunction, bool KeepInlineAsm) {
> 
> "KeepInlineAsm" Any reason we wouldn't want to?

CloneSubModule is meant to help you extract things from modules. You don't want to copy the inline asm every time you extract. 

> +      llvm::make_unique<Module>(M.getModuleIdentifier(), M.getContext());
> 
> Keeping the same identifier?

I'll add a unique suffix - that would be really handy for debugging if we actually logged anything, which we don't yet. :)

> +namespace {
> +
> +const char *JITCallbackFuncName = "call_jit_for_lazy_compile";
> +const char *JITCallbackIndexLabelPrefix = "jit_resolve_";
> +
> +std::array<const char *, 12> X86GPRsToSave = {{
> 
> Might want to throw the X86 bits into its own namespace?

Good call!

Thanks very much for the review Eric. I'll get this stuff tidied up. Look forward to seeing it in tree soon.

- Lang.