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

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:722
+    return true;
+  SmallVector<BasicBlock *, 4> ExitingBlocks;
+  L->getExitingBlocks(ExitingBlocks);
----------------
skatkov wrote:
> skatkov wrote:
> > 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).
> I'm sorry, not a BasicBlock but Loop of course.
> BasicBlock has an utility function getExitEdges.
Yes, that's useful (the one in LoopInfo). Thanks! I haven't seen that function being used anywhere in the code, so it may not be well tested. However, the functionality is exactly what I want and it simplifies the logic below. I'll still extract the LatchExitProbability the way I've done right now, without going through the array.
> I would also consider inserting an assert that latch is an exiting block.
Yup, Latch is an exiting block at this point because Loop Predication requires that form. I'll add an assert here.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:746
+    // predicate.
+    if (ExitingBlockProbability > LatchExitProbability * ProbabilityThreshold)
+      return false;
----------------
skatkov wrote:
> skatkov wrote:
> > It is not a big deal and compiler should do it instead of you but you can hoist LatchExitProbability * ProbabilityThreshold to pre-header :)
> Also please consider renaming of the option ProbabilityThreshold. To me threshold is actually "LatchExitProbability * ProbabilityThreshold". However I might be wrong.
Maybe something like `loop-pred-latch-probability-scale`? 


Repository:
  rL LLVM

https://reviews.llvm.org/D44667