[PATCH] D63378: [ORC] WIP Speculative compilation

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

This is an excellent start!



================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Speculation.h:45-46
+    auto It = Maps.insert({FaceSymbol, Implementations});
+    if (It.second == false)
+      assert(0 && "Source Entities are already tracked for this Symbol?");
+  }
----------------
This should be:

  assert(It.second && "Source Entities are already tracked for this symbol?");


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Speculation.h:54
+    }
+    assert(0 && "Source Entities are not tracked for a Symbol?");
+  }
----------------
hfinkel wrote:
> Don't use `assert(0 && ...`, use llvm_unreachable.
LLVM's coding style does not use braces for single conditional statements, so it should be:

  if (Position != Maps.end())
    return Position->getSecond();

rather than

  if (Position != Maps.end()) {
    return Position->getSecond();
  }

More generally, this function can be reduced to:

  ImplPair getImplFor(SymbolStringPtr StubAddr) {
    auto Position = Maps.find(StubAddr);
    assert(Position != Maps.end() &&
           "Source Entities are not tracked for Symbol?");
    return Position->getSecond();
  }


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Speculation.h:77-83
+    auto It = GlobalSpecMap.find(FAddr);
+    if (It != GlobalSpecMap.end()) {
+      for (auto &Pos : It->getSecond()) {
+        auto SourceEntities = AliaseeImplTable.getImplFor(Pos);
+      }
+    } else
+      assert(0 && "launching compiles for Unexpected Function Address?");
----------------
Applying the coding conventions (no braces for single statements, folding assert conditions into the assert):

  auto It = GlobalSpecMap.find(FAddr);
  assert(It != GlobalSpecMap.end() && "launching compiles for Unexpected function Address?");
  for (auto &Pos : It->getSecond())
    auto SourceEntities = AliaseeImplTable.getImplFor(Pos);

"Pos" is not a very descriptive name here. "Callee" ?

Likewise for SourceEntities. "ImplSymbol"?

SourceEntities is currently unused, but all you should need to do is issue an asynchronous lookup for it with a no-op 'OnComplete' callback. OnComplete takes an error, so you'll have to deal with that, but the usual way is just to log it to the ExecutionSession:

  auto ImplSymbol = AliaseeImplTable.getImplFor(Callee);
  const auto &ImplSymbolName = ImplSymbol.first;
  auto *ImplJD = *ImplSymbol.second;
  ES.lookup({ImplJD, true}, {ImplSymbolName},
            SymbolState::Ready,
            [](Expected<SymbolMap> Result) {
              if (auto Err = Result.takeError())
              ES.reportError(std::move(Err));
            });


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Speculation.h:97-98
+public:
+  using WalkerResultTy = DenseSet<Function *>;
+  using WalkerTy = std::function<WalkerResultTy(const Function &)>;
+
----------------
"Walking" functions describes how you build this set, but not what it is. CalleeSet might be a better name for now?


================
Comment at: llvm/lib/ExecutionEngine/Orc/Speculation.cpp:56
+
+  // reinterpret_cast of Stub Address to i64
+  auto RTFTy = FunctionType::get(Type::getVoidTy(InContext),
----------------
What line is this comment in reference to?


================
Comment at: llvm/lib/ExecutionEngine/Orc/Speculation.cpp:82
+    auto Target = Symbol.first;
+    auto likely = Symbol.second;
+    // Appending Queries on the same symbol but with different callback action
----------------
Variable names should be capitalized, so this should be 'Likely'.


================
Comment at: llvm/lib/ExecutionEngine/Orc/Speculation.cpp:105-107
+  for (auto Name : AR) {
+    Symbols.insert(ESession.intern(Name->getName()));
+  }
----------------
No braces needed here either, since the body is a single statement.


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

https://reviews.llvm.org/D63378