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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:212
+static cl::opt<float>
+    ProbabilityThreshold("loop-predication-threshold-for-profitability",
+                         cl::Hidden, cl::init(2.0));
----------------
I think it is better to add some comment here, what the value of this option actually means.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:722
+    return true;
+  SmallVector<BasicBlock *, 4> ExitingBlocks;
+  L->getExitingBlocks(ExitingBlocks);
----------------
BasicBlock has an utility function getExitEdges.
I propose to use it instead of iteration over ExitingLoops.
This way, your loop will be simplified a lot I guess.

I would also consider inserting an assert that latch is an exiting block.
By definition latch should not require to be an exiting block. But the logic below expects it as I understand...
As I understand the latch is an exiting block due to it is a loop pass and loop is in canonical form. So it is ok but I would still consider inserting an assert... Or alternatively I would find an edge from latch to exit block in an array returned from getExitEdges (this will imply an assert and gives you an input to get probability).


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:746
+    // predicate.
+    if (ExitingBlockProbability > LatchExitProbability * ProbabilityThreshold)
+      return false;
----------------
It is not a big deal and compiler should do it instead of you but you can hoist LatchExitProbability * ProbabilityThreshold to pre-header :)


Repository:
  rL LLVM

https://reviews.llvm.org/D44667