[PATCH] New JIT APIs (ORC)

[Eric Christopher]

On Tue Jan 20 2015 at 6:47:46 PM Lang Hames <lhames at gmail.com> wrote:

> 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?
>
>
That's what I was thinking. IIRC it's also how we bootstrapped up MCJIT.


> > 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.
>
>
Up to you. :)


> > 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.
>
>
Cool deal.


> > 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.
>
>
OK. It seemed like a lot but if they're necessary...


> > 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).
>
>
Mostly what I meant there, you mixed them up a bit. And yeah, what you said
makes sense to me :)


> > 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.
>
>
*nod* Just something to think about.


> > 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).
>

Right. I figured it out, but if you're just looking at the "I want a JIT
for my silly calculator language!" it might not be obvious unless you think
about the various different language rules.


>
> > 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.
>
>
Ah, ok. For some reason I think I missed this in the code. Might want to
double check and probably write some comment as to what happens.


> 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.
>

Agreed. Sounds fun.


>
> > 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.
>
>
Thanks!


> > 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.
>
>
Cool. That sounds good.


> > +      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.
>
>
Oh yuck. OK, this could probably use a giant block comment.


> This whole mess could be avoided if we had an assembly demangler. Perhaps
> that's a sane thing to add?
>
>
Probably not terrible, no.


> > +  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.
>

Thanks.


>
> > +  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.
>

Dylib might make it more confusing in general. I.e. Most things we're
jitting might not be considered a dylib, but rather a single file we're
jitting. (Though I know we use that terminology when talking about jitting
everything as a single shared object in memory). Hmm, I like logical
honestly, just might want to comment on what the logical is.


>
> > +                     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.
>
>
Both ways then. :)

So what's the option actually used for?


> > +      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. :)
>

Heh. And thanks.


>
> > +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.
>

Thanks, it's really great work and I'm glad to see it finally landing.

-eric
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150121/03855349/attachment.html>