[llvm-dev] Orc JIT Layering (original) (raw)

David Blaikie via llvm-dev llvm-dev at lists.llvm.org
Wed Mar 3 15:56:59 PST 2021

On Wed, Mar 3, 2021 at 3:23 PM Lang Hames <lhames at gmail.com> wrote:

Hi Dave, Stefan,

I'm in favor of a re-think of ORC layering. It may take some time to execute though. In the short term I think we could fix this by moving JITSymbol to OrcShared, and making ExecutionEngine depend on OrcShared. Does anyone see any obvious problems with that? If not I can try to make that change some time in the next few weeks.

Looks plausible to me.

Further thoughts, since this discussion reminded me of them: Medium term I'd like to refactor JITSymbol.h: - Replace JITTargetAddress (at least for Orc and JITLink use cases) with OrcTargetAddress, which should be a uint64t wrapper with type safety to ensure that we don't perform bogus arithmetic operations like adding two addresses together. - Rewrite JITSymbolFlags to avoid all the enum casting that it currently requires. - Re-think JITEvaluatedSymbol -- it's very rare that anyone cares about the flags. Maybe ORC lookup should just return a map of OrcTargetAddresses. Longer term I think ORC should be moved out of ExecutionEngine. With ORCv1 removed they're largely separate APIs now. The one sticking point would be RTDyldObjectLinkingLayer, which depends on RuntimeDyld and Orc. I think it would be reasonable to move this into its own library too. -- Lang. On Thu, Mar 4, 2021 at 9:49 AM David Blaikie <dblaikie at gmail.com> wrote:

On Wed, Mar 3, 2021 at 2:35 PM Stefan Gränitz <stefan.graenitz at gmail.com> wrote:

Yes, got it and I also wonder if there's a better low hanging solution.

The underlying question that I wanted to point out was: Would the escape to Support be a one-time solution for JITSymbol or will we see more of the same soon. GDB JIT interface seems to be the next candidate, but OTOH it's quite a special case again. I will have a look at OProfile and Perf implementations for RuntimeDyld to make a better estimation. I guess you'd prefer us to act on it soon now?

I'm not too pressed - Google's unblocked by lumping JITSymbol into Support (we can do this just in the build files without having to move the file around), but the sooner these things are resolved the better to avoid other things layering on top of them, etc. What time frame are you having in mind? Is next week acceptable? (I am more or less ooo for the rest of the week.) Sure

@Peter, @Nico: I've noticed your post-review comments. I will have a look tonight (~10h from now). These should be fixed now. Thanks again for reporting your issues and sorry for the inconvenience. On 03/03/2021 19:40, David Blaikie wrote: To clarify, I'm not sure JITSymbol should be in llvmSupport, but that it's the only place it can be right now that would be correct. Maybe there's some other layering changes (not necessarily introducing a new library, but possibly changing other dependency edges, etc) - but maybe there's already some JIT Stuff in llvmSupport and that's where it should go. It's a simple enough header/wouldn't come at a great cost to include it in Support. On Wed, Mar 3, 2021 at 1:51 AM Stefan Gränitz <stefan.graenitz at gmail.com> wrote: Hi David Thanks for the details. Yes, the layering issue is something we should take care of soon. It also makes trouble for the modules build (see: https://reviews.llvm.org/D95747). I think we should split up the JITSymbol.h and move JITTargetAddress into OrcShared. What remains would be the JITSymbol class. Moving this one to Support sounds like a nice solution to me. However, we have a similar situation with the GDB JIT interface declarations. They should have their own header, yes, but where would we put it? Support too? Not sure about it. Having the definition only in OrcTargetProcess would be acceptable IMHO. The only alternative seems to be an entirely new library (as discussed in the review https://reviews.llvm.org/D97339). What do you think? @Peter, @Nico: I've noticed your post-review comments. I will have a look tonight (~10h from now). Best, Stefan On 03/03/2021 04:57, David Blaikie wrote: Seems one of the latest Orc changes ( https://github.com/llvm/llvm-project/commit/99a6d003edbe97fcb94854547276ffad3382ec1d ) while not itself changing/breaking the layering in LLVM's own build, it has revealed some pre-existing problems with the layering that we'd worked around at Google in a way that isn't viable after this recent change. One immediate/easily observed issue: lib/ExecutionEngine's CMakeLists.txt says it depends on OrcTargetProcess, but OrcTargetProcess includes lib/ExecutionEngine/JITSymbol.h The only common dependency for all the uses of JITSymbol.h seems to be llvm/Support (ie: without introducing new dependencies or new libraries, JITSymbol.h would need to be moved to llvm/Support to fix this particular dependency cycle/issue) We do have a bunch of other workarounds for Orc layering in the Google internal build system too - so perhaps I can enumerate some/all of the issues here, as it might be best to take a holistic approach to fixing these issues. Let's see what I can document/figure out... ExecutionEngine/Orc -> ExecutionEngine ExecutionEngine/Interpreter -> ExecutionEngine ExecutionEngine/RuntimeDyld -> ExecutionEngine ExecutionEngine/IntelJITEvents -> ExecutionEngine ExecutionEngine/OProfileJIT -> ExecutionEngine ExecutionEngine/PerfJITEvents -> ExecutionEngine ExecutionEngine/MCJIT -> ExecutionEngine And there's actually no #includes in ExecutionEngine that reference those libraries, so that's pretty good. It is this CMakeLists.txt dependency from ExecutionEngine to OrcTargetProcess. Which happens without a #include: _$ grep -r "void jitdebugregistercode" llvm/ llvm/lib/ExecutionEngine/GDBRegistrationListener.cpp: extern "C" *void _jitdebugregistercode*(); llvm/lib/ExecutionEngine/Orc/TargetProcess/JITLoaderGDB.cpp:LLVMATTRIBUTENOINLINE _void _jitdebugregistercode() {_ Would be better if this wasn't declared arbitrarily (instead, if it was declared in a header and defined as usual, the circular dependence would be more clear, I think?) - but either way, the circular dependency needs to be fixed. - Dave

-- https://flowcrypt.com/pub/stefan.graenitz@gmail.com -- https://flowcrypt.com/pub/stefan.graenitz@gmail.com -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210303/c2d4ac6d/attachment.html>

More information about the llvm-dev mailing list