[PATCH] D89541: [IRCE] Do not transform is loop has small number of iterations

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:120
+static cl::opt<unsigned> MinEstimatedIterations("irce-min-estimated-iters",
+                                                cl::Hidden, cl::init(3));
+
----------------
I think the name should hint that this is profile-based, e.g. "MinRuntimeLoopIterations" or so. We also might want to have another boolean flag to turn this check off, because some users of IRCE might not have a profile.


================
Comment at: llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:244
+      llvm::Optional<llvm::function_ref<llvm::BlockFrequencyInfo &()> >;
+  GetBFIFunc GetBFI;
+
----------------
Why not `Optional<BlockFrequencyInfo>`?


================
Comment at: llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1791
   bool Changed = false;
-
+  bool CFGChanged = false;
   for (const auto &L : LI) {
----------------
If you stop updating this thing after pre-opt preparations, does it make sense to limit its scope? There is a temptation to use `CFGChanged` variable in the end to not drop some analysis if it did not happen.


================
Comment at: llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1909
+        (hFreq / phFreq < MinEstimatedIterations)) {
+      LLVM_DEBUG(dbgs() << "irce: could not prove profit: "
+                        << "freq estimated number iterations is "
----------------
profitability


================
Comment at: llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1910
+      LLVM_DEBUG(dbgs() << "irce: could not prove profit: "
+                        << "freq estimated number iterations is "
+                        << (hFreq / phFreq) << "\n";);
----------------
"the estimated number of iterations basing on frequency info"?


================
Comment at: llvm/test/Transforms/IRCE/low-iterations.ll:1
+; RUN: opt -verify-loop-info -irce-print-changed-loops -passes=irce < %s 2>&1 | FileCheck %s
+;
----------------
Pass the frequency parameter value directly rather than rely on the default value.


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

https://reviews.llvm.org/D89541