[PATCH] D63378: [ORC] WIP Speculative compilation

[Lang Hames via Phabricator via llvm-commits]

lhames accepted this revision.
lhames added a comment.
This revision is now accepted and ready to land.

Minor style nits aside, this looks great. Once you've addressed them (and run clang-format over the patch) feel free to commit this to the mainline and continue development there. :)



================
Comment at: llvm/examples/SpeculativeJIT/SpeculativeJIT.cpp:198
+}
\ No newline at end of file

----------------
There should be a newline here.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/LazyReexports.h:23
 
+#include "llvm/Support/FileOutputBuffer.h"
 namespace llvm {
----------------
Is this FileOutputBuffer include needed?


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/LazyReexports.h:188
               IndirectStubsManager &ISManager, JITDylib &SourceJD,
-              SymbolAliasMap CallableAliases, VModuleKey K = VModuleKey()) {
+              SymbolAliasMap CallableAliases, ImplSymbolMap *SrcJDLoc,
+              VModuleKey K = VModuleKey()) {
----------------
The SrcJDLoc parameter should have a default value of nullptr, so that people who are not using speculation can ignore it.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Speculation.h:54
+    else
+      return {};
+  }
----------------
It's best to use "return None" rather than "return {}". I've seen some older compilers struggle with the latter.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Speculation.h:71
+                               SymbolNameSet likelySymbols) {
+    std::lock_guard<std::mutex> lockit(ConcurrentAccess);
+    GlobalSpecMap.insert({ImplAddr, std::move(likelySymbols)});
----------------
The LLVM style guide says that variable names should start with a capital letter, so lockit should be Lockit.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Speculation.h:80
+    {
+      std::lock_guard<std::mutex> lockit(ConcurrentAccess);
+      auto It = GlobalSpecMap.find(FAddr);
----------------
Lockit should start with a capital letter here too.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Speculation.h:114
+  Speculator &operator=(Speculator &&) = delete;
+  virtual ~Speculator() {}
+
----------------
Speculator doesn't seem to have any virtual methods. Are you intending for this to be subclassed? If not, you can remove the virtual destructor too.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Speculation.h:163-170
+  template <
+      typename AnalysisTy,
+      typename std::enable_if<
+          std::is_base_of<AnalysisInfoMixin<AnalysisTy>, AnalysisTy>::value,
+          bool>::type = true>
+  void registerAnalysis() {
+    FAM.registerPass([]() { return AnalysisTy(); });
----------------
I think this should be able to be removed now?


================
Comment at: llvm/lib/ExecutionEngine/Orc/SpeculateAnalyses.cpp:76
+  // ignoring number of direct calls in a BB
+  auto topk = numBBToGet(BBFreqs.size());
+
----------------
topk should start with a capital letter. 


================
Comment at: llvm/lib/ExecutionEngine/Orc/Speculation.cpp:30
+  assert(SrcJD && "Tracking on Null Source .impl dylib");
+  std::lock_guard<std::mutex> lockit(ConcurrentAccess);
+  for (auto &I : ImplMaps) {
----------------
lockit should start with a capital letter. 


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

https://reviews.llvm.org/D63378