[llvm-dev] Orc JIT Layering

[Lang Hames via llvm-dev]

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.

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/20210304/20cf6a3a/attachment.html>