[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing
Lang Hames via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 17 19:26:34 PST 2021
lhames added inline comments.
================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:908
+CodeGenerator *CodeGenAction::getCodeGenerator() const {
+ return BEConsumer->getCodeGenerator();
----------------
v.g.vassilev wrote:
> sgraenitz wrote:
> > v.g.vassilev wrote:
> > > @rjmccall, we were wondering if there is a better way to ask CodeGen to start a new module. The current approach seems to be drilling hole in a number of abstraction layers.
> > >
> > > In the past we have touched that area a little in https://reviews.llvm.org/D34444 and the answer may be already there but I fail to connect the dots.
> > >
> > > Recently, we thought about having a new FrontendAction callback for beginning a new phase when compiling incremental input. We need to keep track of the created objects (needed for error recovery) in our Transaction. We can have a map of `Transaction*` to `llvm::Module*` in CodeGen. The issue is that new JITs take ownership of the `llvm::Module*` which seems to make it impossible to support jitted code removal with that model (cc: @lhames, @rsmith).
> > When compiling incrementally, doeas a "new phase" mean that all subsequent code will go into a new module from then on? How will dependencies to previous symbols be handled? Are all symbols external?
> >
> > > The issue is that new JITs take ownership of the llvm::Module*
> >
> > That's true, but you can still keep a raw pointer to it, which will be valid at least as long as the module wasn't linked. Afterwards it depends on the linker:
> > * RuntimeDyld can return ownership of the object's memory range via `NotifyEmittedFunction`
> > * JITLink provides the `ReturnObjectBufferFunction` for the same purpose
> >
> > > seems to make it impossible to support jitted code removal with that model
> >
> > Can you figure out what symbols are affected and remove these? A la: https://github.com/llvm/llvm-project/blob/13f4448ae7db1a47/llvm/include/llvm/ExecutionEngine/Orc/Core.h#L1020
> >
> > I think @anarazel has ported a client with code removal to OrcV2 successfully in the past. Maybe there's something we can learn from it.
> > When compiling incrementally, doeas a "new phase" mean that all subsequent code will go into a new module from then on? How will dependencies to previous symbols be handled? Are all symbols external?
>
> There is some discussion on this here https://reviews.llvm.org/D34444#812418
>
> I think the relevant bit is that 'we have just one ever growing TU [...] which we send to the RuntimeDyLD allowing only JIT to resolve symbols from it. We aid the JIT when resolving symbols with internal linkage by changing all internal linkage to external (We haven't seen issues with that approach)'.
>
> >
> > > The issue is that new JITs take ownership of the llvm::Module*
> >
> > That's true, but you can still keep a raw pointer to it, which will be valid at least as long as the module wasn't linked.
>
> That was my first implementation when I upgraded cling to llvm9 where the `shared_ptr`s went to `unique_ptr`s. This was quite problematic for many of the use cases we support as the JIT is somewhat unpredictable to the high-level API user.
>
>
> >Afterwards it depends on the linker:
> > * RuntimeDyld can return ownership of the object's memory range via `NotifyEmittedFunction`
> > * JITLink provides the `ReturnObjectBufferFunction` for the same purpose
> >
>
> That's exactly what we ended up doing (I would like to thank Lang here who gave a similar advice).
>
> > > seems to make it impossible to support jitted code removal with that model
> >
> > Can you figure out what symbols are affected and remove these? A la: https://github.com/llvm/llvm-project/blob/13f4448ae7db1a47/llvm/include/llvm/ExecutionEngine/Orc/Core.h#L1020
> >
> > I think @anarazel has ported a client with code removal to OrcV2 successfully in the past. Maybe there's something we can learn from it.
>
> Indeed. That's not yet on my radar as seemed somewhat distant in time.
>
> Recently, we thought about having a new FrontendAction callback for beginning a new phase when compiling incremental input. We need to keep track of the created objects (needed for error recovery) in our Transaction. We can have a map of Transaction* to llvm::Module* in CodeGen. The issue is that new JITs take ownership of the llvm::Module* which seems to make it impossible to support jitted code removal with that model (cc: @lhames, @rsmith).
In the new APIs, in order to enable removable code, you can associate Modules with ResourceTrackers when they're added to the JIT. The ResourceTrackers then allow for removal. Idiomatic usage looks like:
auto Mod = /* create module */;
auto RT = JD.createResourceTracker();
J.addModule(RT, std::move(Mod));
//...
if (auto Err = RT.remove())
/* handle Err */;
> we have just one ever growing TU [...] which we send to RuntimeDyld...
So is a TU the same as an llvm::Module in this context? If so, how do you reconcile that with the JIT taking ownership of modules? Are you just copying the Module each time before adding it?
> We need to keep track of the created objects (needed for error recovery) in our Transaction.
Do you need the Module* for error recovery? Or just the Decls?
================
Comment at: clang/lib/Interpreter/IncrementalExecutor.cpp:29-51
+ using namespace llvm::orc;
+ llvm::ErrorAsOutParameter EAO(&Err);
+ auto JitOrErr = LLJITBuilder().create();
+ if (auto Err2 = JitOrErr.takeError()) {
+ Err = std::move(Err2);
+ return;
+ }
----------------
I think this can be shortened to:
using namespace llvm::orc;
llvm::ErrorAsOutParameter EAO(&Err);
if (auto JitOrErr = LLJITBuilder.create())
Jit = std::move(*JitOrErr);
else {
Err = JitOrErr.takeError();
return;
}
const auto &DL = Jit->getDataLayout();
if (auto PSGOrErr = DynamicLibrarySearchGenerator::GetForCurrentProcess(DL.getGlobalPrefix()))
Jit->getMainJITDylib().addGenerator(std::move(*PSGOrErr));
else {
Err = PSGOrErr.takeError();
return;
}
You don't need the call to `llvm::sys::DynamicLibrary::LoadLibraryPermanently(nullptr);` any more: DynamicLibrarySearchGenerator::GetForCurrentProcess does that for you.
================
Comment at: clang/lib/Interpreter/IncrementalExecutor.cpp:56-59
+llvm::Error IncrementalExecutor::addModule(std::unique_ptr<llvm::Module> M) {
+ llvm::orc::ThreadSafeContext TSCtx(std::make_unique<llvm::LLVMContext>());
+ return Jit->addIRModule(llvm::orc::ThreadSafeModule(std::move(M), TSCtx));
+}
----------------
This doesn't look right. The ThreadSafeContext has to contain the LLVMContext for the module, but here you're creating a new unrelated ThreadSafeContext.
================
Comment at: clang/lib/Interpreter/IncrementalExecutor.h:36
+ llvm::Error addModule(std::unique_ptr<llvm::Module> M);
+ llvm::Error runCtors() const;
+};
----------------
v.g.vassilev wrote:
> sgraenitz wrote:
> > v.g.vassilev wrote:
> > > teemperor wrote:
> > > > Should we maybe merge `runCtors` and `addModule`? Not sure if there is a use case for adding a Module but not running Ctors. Also documentation.
> > > The case we have is when there is no JIT -- currently we have such a case in IncrementalProcessingTest I think. Another example, which will show up in future patches, is the registration of atexit handlers. That is, before we `runCtors` we make a pass over the LLVM IR and collect some specific details and (possibly change the IR and then run).
> > >
> > > I'd rather keep it separate for now if that's okay.
> > > Should we maybe merge runCtors and addModule?
> >
> > +1 even though there may be open questions regarding incremental initialization.
> >
> > > The case we have is when there is no JIT -- currently we have such a case in IncrementalProcessingTest
> >
> > Can you run anything if there is no JIT? I think what you have in `IncrementalProcessing.EmitCXXGlobalInitFunc` is `getGlobalInit(llvm::Module*)`, which checks for symbol names with a specific prefix.
> >
> > > before we runCtors we make a pass over the LLVM IR and collect some specific details and (possibly change the IR and then run).
> >
> > The idiomatic solution for such modifications would use an IRTransformLayer as in:
> > https://github.com/llvm/llvm-project/blob/13f4448ae7db1a47/llvm/examples/OrcV2Examples/LLJITWithOptimizingIRTransform/LLJITWithOptimizingIRTransform.cpp#L108
> >
> > > Another example, which will show up in future patches, is the registration of atexit handlers
> >
> > `atexit` handlers as well as global ctors/dtors should be covered by LLJIT PlatformSupport. The LLJITBuilder will inject a GenericLLVMIRPlatformSupport instance by default:
> > https://github.com/llvm/llvm-project/blob/13f4448ae7db1a47/llvm/lib/ExecutionEngine/Orc/LLJIT.cpp#L125
> >
> > It's not as comprehensive as e.g. the MachO implementation, but should be sufficient for your use-case as you have IR for all your JITed code. (It would NOT work if you cached object files, reloaded them in a subsequent session and wanted to run their ctors.) So, your below call to `initialize()` should do it already.
> >
> > > Should we maybe merge runCtors and addModule?
> >
> > +1 even though there may be open questions regarding incremental initialization.
> >
> > > The case we have is when there is no JIT -- currently we have such a case in IncrementalProcessingTest
> >
> > Can you run anything if there is no JIT? I think what you have in `IncrementalProcessing.EmitCXXGlobalInitFunc` is `getGlobalInit(llvm::Module*)`, which checks for symbol names with a specific prefix.
>
> Yes, I'd think such mode is useful for testing but also for other cases where the user is handed a Transaction* and allowed to make some modification before processing the `llvm::Module`
>
> >
> > > before we runCtors we make a pass over the LLVM IR and collect some specific details and (possibly change the IR and then run).
> >
> > The idiomatic solution for such modifications would use an IRTransformLayer as in:
> > https://github.com/llvm/llvm-project/blob/13f4448ae7db1a47/llvm/examples/OrcV2Examples/LLJITWithOptimizingIRTransform/LLJITWithOptimizingIRTransform.cpp#L108
>
> That looks very nice. It assumes the JIT is open to the users, here we open only the `llvm::Module` (I am not arguing if that's a good idea in general).
>
> >
> > > Another example, which will show up in future patches, is the registration of atexit handlers
> >
> > `atexit` handlers as well as global ctors/dtors should be covered by LLJIT PlatformSupport. The LLJITBuilder will inject a GenericLLVMIRPlatformSupport instance by default:
> > https://github.com/llvm/llvm-project/blob/13f4448ae7db1a47/llvm/lib/ExecutionEngine/Orc/LLJIT.cpp#L125
>
> Does that give me control over when the `atexit` handlers are called? Can the interpreter call them at its choice?
>
> >
> > It's not as comprehensive as e.g. the MachO implementation, but should be sufficient for your use-case as you have IR for all your JITed code. (It would NOT work if you cached object files, reloaded them in a subsequent session and wanted to run their ctors.) So, your below call to `initialize()` should do it already.
> >
>
>
>> Should we maybe merge runCtors and addModule?
> +1 even though there may be open questions regarding incremental initialization.
In the long term constructors should be run via the Orc runtime (currently planned for initial release in LLVM 13 later this year). I like the idea of keeping "add module" and "run initializers" as two separate steps, with initializers being run only when you execute a top level expression. It would allow for workflows like this:
interpreter% :load a.cpp
interpreter% :load b.cpp
where an initializer in a.cpp depends on code in b.cpp. It would also allow for defining constructors with forward references in the REPL itself.
The Orc runtime is currently focused on emulating the usual execution environment: The canonical way to execute initializers is by calling jit_dlopen on the target JITDylib. I think the plan should be to generalize this behavior (either in the jit_dlopen contract, or by introducing a jit_dlopen_repl function) to allow for repeated calls to dlopen, with each subsequent dlopen call executing any discovered-but-not-yet-run initializers.
>> Does that give me control over when the atexit handlers are called? Can the interpreter call them at its choice?
>
> It's not as comprehensive as e.g. the MachO implementation, but should be sufficient for your use-case as you have IR for all your JITed code. (It would NOT work if you cached object files, reloaded them in a subsequent session and wanted to run their ctors.) So, your below call to initialize() should do it already.
Yep -- initialize should run the constructors, which should call cxa_atexit. The cxa_atexit calls should be interposed by GenericLLVMIRPlatform, and the atexits run when you call LLJIT::deinitialize on the JITDylib. There are some basic regression tests for this, but it hasn't been stress tested yet.
GenericLLVMIRPlatform should actually support initializers in cached object files that were compiled from Modules added to LLJIT: The platform replaces llvm.global_ctors with an init function with a known name, then looks for that name in objects loaded for the cache. At least that was the plan, I don't recall whether it has actually been tested. What definitely doesn't work is running initializers in objects produced outside LLJIT. That will be fixed by JITLink/ELF and the Orc Runtime though (and already works for MachO in the runtime prototype).
================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:155-159
+ IncrementalAction *IncrAct = static_cast<IncrementalAction*>(Act);
+ FrontendAction *WrappedAct = IncrAct->getWrapped();
+ if (!WrappedAct->hasIRSupport())
+ return nullptr;
+ return static_cast<CodeGenAction*>(WrappedAct)->getCodeGenerator();
----------------
Wherever this CodeGenAction is created it's probably the missing piece of the ThreadSafeContext puzzle: CodeGenAction's constructor takes an LLVMContext*, creating a new LLVMContext if the argument is null. In your system (at least to start with) I guess you will want to create one ThreadSafeModule associated with the interpreter and pass a pointer to that context to all CodeGenActions.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96033/new/
https://reviews.llvm.org/D96033
More information about the cfe-commits
mailing list