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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov accepted this revision.
skatkov added a comment.
This revision is now accepted and ready to land.

LGTM with some comments.



================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:215
+// This value should be greater than 1 for a sane profitability check.
+static cl::opt<float>
+    LatchExitProbabilityScale("loop-predication-latch-probability-scale",
----------------
cl::opt has a way to add a description for the option: cl::desc("")
I would suggest always to provide a description which is printed in help message.
The notice about >=1 and ignoring the value <1 can be also added to desc.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:726
+    return true;
+  SmallVector<std::pair<const BasicBlock *, const BasicBlock *>, 8> ExitEdges;
+  L->getExitEdges(ExitEdges);
----------------
new line?


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:750
+  if (LatchExitProbabilityScale < 1)
+    LatchExitProbabilityScale = 1.0;
+  const auto LatchProbabilityThreshold =
----------------
DEBUG(dbgs() with message that we ignore user settings?
It is not a good idea to update the value of opt.  I would suggest to use local variable for that.


Repository:
  rL LLVM

https://reviews.llvm.org/D44667