[llvm-dev] Orc JIT Layering

[Stefan Gränitz via llvm-dev]

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? What time frame are you
having in mind? Is next week acceptable? (I am more or less ooo for the
rest of the week.)

> @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 <mailto: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
>     <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
>     <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
>>     <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 __jit_debug_register_code" llvm/
>>
>>     llvm/lib/ExecutionEngine/GDBRegistrationListener.cpp:  extern "C"
>>     *void __jit_debug_register_code*();
>>
>>     llvm/lib/ExecutionEngine/Orc/TargetProcess/JITLoaderGDB.cpp:LLVM_ATTRIBUTE_NOINLINE
>>     *void __jit_debug_register_code*() {
>>
>>
>>     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>
>
-- 
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/0c0a187e/attachment.html>