[PATCH] D74300: [ORC] Add generic initializer/deinitializer support.

[Stefan Gränitz via Phabricator via llvm-commits]

sgraenitz added a comment.

Hi Lang, I had a look at the first half of your changes. So far it looks really great!
A few mini notes inline and a larger one that's semi-related. I will continue with the second half and run this by the ThinLtoJIT example asap.



================
Comment at: llvm/lib/ExecutionEngine/Orc/CompileOnDemandLayer.cpp:155
+
+    // Promote globals and define new materializing symbols.
+    SmallVector<GlobalValue *, 16> PromotedGlobals;
----------------
Is GV promotion here related to the initializers change? Is it always necessary or can we skip it in the whole module partitioning case?


================
Comment at: llvm/lib/ExecutionEngine/Orc/CompileOnDemandLayer.cpp:168
+      else if (GV.getName().startswith("\01L"))
+        GV.setName("__" + GV.getName().substr(1) + "." + Twine(++NextPromoId));
+      else if (GV.hasLocalLinkage())
----------------
This is an ObjC special case right? Can we have a comment for it?


================
Comment at: llvm/lib/ExecutionEngine/Orc/Core.cpp:1130
       // Move symbol to the emitted state.
+
       assert(SymEntry.getState() == SymbolState::Resolved &&
----------------
Unintended newline I guess.


================
Comment at: llvm/lib/ExecutionEngine/Orc/Core.cpp:1958
+              CompoundErr =
+                  joinErrors(std::move(CompoundErr), Result.takeError());
+          }
----------------
The typical ErrorList vs. reportError() case as discussed in D72176. Isn't it a similar situation as with the OnComplete handler in ReExportsMaterializationUnit::materialize()? That one reports the incoming error.

While looking at this, should it be the handler's responsibility at all to think about incoming errors? Is there a deeper reason for this? Otherwise, couldn't the NotifyComplete callback bring a Optional<SymbolMap> instead of an Expected<SymbolMap>? IIUC AsynchronousSymbolQuery has two distinct code paths for success vs. error already: handleComplete() and handleFailed(). The latter could call NotifyComplete(None) and handle the error independently. However, it's not directly connected to this review.


================
Comment at: llvm/lib/ExecutionEngine/Orc/LLJIT.cpp:37
+
+/// Adds helper function decls and wrapper functions tha call the helper with
+/// some additional prefix arguments.
----------------
Typo: tha -> that
This is how far I got.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74300/new/

https://reviews.llvm.org/D74300