[PATCH] D44667: [LoopPredication] Add profitability check based on BPI

[Anna Thomas via Phabricator via llvm-commits]

anna marked 2 inline comments as done.
anna added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:726
+  SmallVector<BasicBlock *, 4> ExitingBlocks;
+  L->getExitingBlocks(ExitingBlocks);
+  auto *LatchBlock = L->getLoopLatch();
----------------
skatkov wrote:
> you do not need ExitingBlocks more.
stale line will remove.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:736
+      BPI->getEdgeProbability(LatchBlock, LatchBrExitIdx);
+  SmallVector<std::pair<const BasicBlock*, const BasicBlock*>, 8> ExitEdges;
+  L->getExitEdges(ExitEdges);
----------------
skatkov wrote:
> Please consider re-ordering the lines in this piece of code (it is a bit difficult to read it).
> 
> I would suggest to move all code related to getting edge probability below to computation of LatchProbabilityThreshold (to me it is connected parts). Additionally you will have an earlier bail out due to more than one exit and you do not need to do anything for latch in this case.
> 
> And separate by new line the code related to LatchProbabilityThreshold and getting ExitEdges.
ok.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:747
+  // predicate.
+  for (const auto &ExitEdge : ExitEdges) {
+    BranchProbability ExitingBlockProbability =
----------------
skatkov wrote:
> Just a side comment... Instead of "for loop" you can consider "any_of" usage in this case. However I do not know whether it looks better :) So it is up to you.
since there's an extra computation of probability here, I'm not sure if  any_of is better :)


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:748
+  for (const auto &ExitEdge : ExitEdges) {
+    BranchProbability ExitingBlockProbability =
+        BPI->getEdgeProbability(ExitEdge.first, ExitEdge.second);
----------------
skatkov wrote:
> You still need to skip latch exiting basic block.
you don't need to skip it. The check at line 750 will never be true for latch exit basic block (exitingblock probability > latchExitingBlockProbability). I intentionally ignored checking for latch exiting basic block, it saves us a check for every exiting edge...


Repository:
  rL LLVM

https://reviews.llvm.org/D44667