[PATCH] D66399: [ORCv2] - New Speculate Query Implementation

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/SpeculateAnalyses.h:40
+class BlockFreqQuery : public SpeculateQuery {
+private:
+  size_t numBBToGet(size_t);
----------------
You can remove this 'private' since it's implicit


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/SpeculateAnalyses.h:53
+class SequenceBBQuery : public SpeculateQuery {
+private:
+  struct WalkDirection {
----------------
no need for 'private' here, when a class members are private by default


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/SpeculateAnalyses.h:55
+  struct WalkDirection {
+  public:
+    bool Upward, Downward;
----------------
no need for 'public' in a struct where it's public by default


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/SpeculateAnalyses.h:59
+    bool CallerBlock;
+    WalkDirection() : Upward(true), Downward(true), CallerBlock(false) {}
+  };
----------------
Perhaps use non-static data member initializers instead of a ctor here? (add initializers "= true", etc to the member declarations directly, and remove this ctor)


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Speculation.h:85-86
+      if (It == GlobalSpecMap.end())
         return;
       else
         CandidateSet = It->getSecond();
----------------
Remove else-after-return ( https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return )


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Speculation.h:114
   Speculator &operator=(Speculator &&) = delete;
-  ~Speculator() {}
+  ~Speculator() = default;
+
----------------
Does this dtor need to be explicitly declared? Or could this line be deleted?


================
Comment at: llvm/lib/ExecutionEngine/Orc/SpeculateAnalyses.cpp:72-73
+bool SpeculateQuery::isStraightLine(const Function &F) {
+  if (F.getBasicBlockList().size() == 1)
+    return true;
+  return llvm::all_of(F.getBasicBlockList(), [](const BasicBlock &BB) -> bool {
----------------
Does this capture any case that isn't captured by the all_of below? Perhaps this special case could be removed?


================
Comment at: llvm/lib/ExecutionEngine/Orc/SpeculateAnalyses.cpp:74
+    return true;
+  return llvm::all_of(F.getBasicBlockList(), [](const BasicBlock &BB) -> bool {
+    return BB.getSingleSuccessor() ? true : false;
----------------
You can drop the "-> bool" part - it's implicit & probably clear enough from the implementation that the return type is bool.


================
Comment at: llvm/lib/ExecutionEngine/Orc/SpeculateAnalyses.cpp:75
+  return llvm::all_of(F.getBasicBlockList(), [](const BasicBlock &BB) -> bool {
+    return BB.getSingleSuccessor() ? true : false;
+  });
----------------
"x ? true : false" seems redundant. Perhaps "return BB.getSingleSuccessor() != nullptr;" would be OK/better?


================
Comment at: llvm/lib/ExecutionEngine/Orc/SpeculateAnalyses.cpp:135-136
+  if (TotalBlocks == 1)
+    return TotalBlocks;
+  else
+    return TotalBlocks / 2;
----------------
Remove else-after-return


================
Comment at: llvm/lib/ExecutionEngine/Orc/SpeculateAnalyses.cpp:146
+  for (auto &Block : AllBlocks) {
+    auto FItr = std::find(BBList.begin(), BBList.end(), &Block);
+    // found it
----------------
Can probably use llvm::find(BBList, &Block) rather than having to pass begin/end here


================
Comment at: llvm/lib/ExecutionEngine/Orc/SpeculateAnalyses.cpp:149
+    if (FItr != BBList.end())
+      RearrangedBBSet.push_back(*FItr);
+  }
----------------
*FItr == &Block here, right? Perhaps it'd be more clear to use &Block here, then - so it's clear there's nothing being accessed in BBList - it's only used for a lookup test?

(& maybe there's a neater way to write that - ah, llvm::is_contained?)


================
Comment at: llvm/lib/ExecutionEngine/Orc/SpeculateAnalyses.cpp:210-211
+    if (!Itr->second.Downward)
+      return;
+    else
+      Itr->second.Downward = false;
----------------
Remove else-after-return


================
Comment at: llvm/lib/ExecutionEngine/Orc/SpeculateAnalyses.cpp:222-224
+      VisitedBlocks.insert(std::make_pair(AtBB, BlockHint));
+    } else
+      VisitedBlocks.insert(std::make_pair(AtBB, BlockHint));
----------------
Perhaps these two calls to VisitedBlocks could be coalesced?

  if (llvm::is_contained(CallerBlocks, AtBB))
    BlockHint.CallerBlock = true;
  VisitedBlocks.insert(std::make_pair(AtBB, BlockHint));


================
Comment at: llvm/lib/ExecutionEngine/Orc/SpeculateAnalyses.cpp:235
+  // occurs in first position.
+  for (auto I = BackEdgesInfo.begin(), E = BackEdgesInfo.end(); I != E; ++I)
+    if (I->first == AtBB)
----------------
Should this use a range-based for loop?


================
Comment at: llvm/lib/ExecutionEngine/Orc/SpeculateAnalyses.cpp:239
+
+  for (; PIt != EIt; ++PIt)
+    if (BPI->isEdgeHot(AtBB, *PIt) && !SuccSkipNodes.count(*PIt))
----------------
Could this use a range-based for loop too?


================
Comment at: llvm/lib/ExecutionEngine/Orc/SpeculateAnalyses.cpp:269
+                decltype(BBFreqs)::const_reference BBS) {
+               return BBF.second > BBS.second ? true : false;
+             });
----------------
Skip the "? true : false" here, just "return BBF.second > BBS.second;" - that's already a boolean expression.


================
Comment at: llvm/lib/ExecutionEngine/Orc/SpeculateAnalyses.cpp:281-282
+
+  for (size_t i = 0; i < NHotBlocks; ++i) {
+    const BasicBlock *BB = BBFreqs[i].first;
+    traverseToEntryBlock(BB, CallerBlocks, BackEdgesInfo, BPI, VisitedBlocks);
----------------
Possibly using range-based for loop here (maybe using an arrayref sliced down to NHotBlocks)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66399