[llvm-dev] Orc JIT Layering

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 uint64_t 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 __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
>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210303/c2d4ac6d/attachment.html>