Make copy of ORC SimpleCompiler functor for LLILCCompiler

[russellhadley]

New functor will be extended to implement LLILC specific phase lists.

[russellhadley]

Here you go @pgavlin. Just a copy.

[pgavlin]

LGTM, thanks!

[JosephTremoulet]

What will the LLILC-specific passes be?

[JosephTremoulet]

Why is this in the llvm::orc namespace? I'd prefer a llilc namespace (or just use root like we do for most of our code) for LLILC code. (fwiw, I feel the same way about our EEMemoruManager)

[pgavlin]

AFAIK the only pass that is LLILC-specific is a TargetLibraryAnalysis pass to provide information about the stlib functions that are available on CoreCLR. We'll also want to be able to customize the set (and perhaps order) of passes that we run, even for those passes that are not LLILC-specific.

[JosephTremoulet]

We'll also want to be able to customize the set (and perhaps order) of passes that we run, even for those passes that are not LLILC-specific.

I don't follow how this change helps with that; the passes are still added opaquely in the call to TM.addPassesToEmitMC. Customizing the set/order of passes would need to take the form of changes to the TargetMachine code conditional on CoreCLR environment, that we'd upstream, wouldn't they?

a TargetLibraryAnalysis pass to provide information about the stlib functions that are available on CoreCLR

And is that something that needs to be run in the same PassManager as the other passes (otherwise we could use a separate pass/layer before the IRCompileLayer)? Is this something that other JITs are going to need; should we look into upstreaming code to add the TargetLibraryAnalysis pass in TargetMachine::addPassesToEmitMC and/or adding a hook to SimpleCompiler for seeding the PassManager?

Just hate to see copied code...

[richardlford]

For LLILC we use \brief rather than @brief. And similarly below.

[pgavlin]

I don't follow how this change helps with that; the passes are still added opaquely in the call to TM.addPassesToEmitMC. Customizing the set/order of passes would need to take the form of changes to the TargetMachine code conditional on CoreCLR environment, that we'd upstream, wouldn't they?

They are at the moment, but I imagine that this will not be the case in the future. FWIW, all of the other LLVM-based compilers I've peeked at (e.g. Clang, Rust, Loci, etc.) manually manage passes.

And is that something that needs to be run in the same PassManager as the other passes (otherwise we could use a separate pass/layer before the IRCompileLayer)?

Yes. Its purpose is to make the particulars of the target stdlib available to later passes in the same pass manager.

Is this something that other JITs are going to need; should we look into upstreaming code to add the TargetLibraryAnalysis pass in TargetMachine::addPassesToEmitMC and/or adding a hook to SimpleCompiler for seeding the PassManager?

It might be something that other JITs will need--the more general case is certainly something other JITs might need--but I did not get much of a response the last time I asked llvm-dev about this. Could be worth trying again, however.

[JosephTremoulet]

Thanks for the extra context. It seems to me that having a way to create an IRCompileLayer with a custom set of passes would be a useful orc utility, I don't know why the pass setup is currently bundled with the memory stream / object file maintenance, I do think it would be worth asking if there's interest in that (maybe asking as an orc question might get more response than asking as an ExecutionEngine question did?).

Separately, it might be a good idea to add the two safepoint-related passes in this pass manager instead of their own.

[russellhadley]

Hey @JosephTremoulet, the namespace was to make the copy easier. Each of the times in the functor method would need a 'llvm::" prefix for compile. And I didn't want to add the 'using' statement since this is a header. Stepping back a bit this is the best way I could see to get ahold of the phase list. I agree that the current statepoint code needs to be refactored. (thought there are a lot of precedents for separate pass managers.) My particular need for this is to inject a pass late to inject debug interrupts (int 3) to model the same debugging loop that RyuJit has. Looking ahead we will need, as you say, the ability to add a particular set of passes to the pass manager based on passed flags from the runtime. I'm expecting this functor to be the place that those decisions get made.

[JosephTremoulet]

Each of the times in the functor method would need a 'llvm::" prefix for compile

That seems cleaner to me despite the extra typing. It's also what we do in our other header files. (aside: it's not clear why this method body is in the header file; the whole class could be in llilc.cpp or even be compiler-generated by passing a lambda to the IRCompileLayer constructor). Maybe putting LLILC code in a llvm::llilc namespace would make sense and save keystrokes? (I realize this class already has LLILC in its name and own't conflict with anything in LLVM; but I'm also wanting to pick a direction here since what we have now is different conventions in different LLILC files w.r.t. namespaces depending who wrote the file).

Looking ahead we will need, as you say, the ability to add a particular set of passes to the pass manager based on passed flags from the runtime. I'm expecting this functor to be the place that those decisions get made

Sure, at this point I'm just wondering why we want a copy of the boilerplate code for allocating/parsing the object file and its buffer and error reporting. Instead we could e.g. factor an inner functor out of SimpleCompiler for just the pass setup. It would let us reduce the client code to just a lambda that adds whatever passes it wants, and I'd imagine other clients will come along that want the same thing, no?

[AndyAyersMS]

@russellhadley @JosephTremoulet I'm playing catch-up here. Seems like we didn't really close on Joe's feedback...? Was there some discussion I missed?