[PATCH] D125990: [LSR] Fix bug for optimizing unused IVs to final values

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:113
+               clEnumValN(UnusedIndVar, "unusedindvar",
+                          "only replace unused induction variables"),
                clEnumValN(NoHardUse, "noharduse",
----------------
UnusedIndVar seems to subsume OnlyCheapRepl. If this not tautological (see comment below), consider reflecting that in the description. Suggestion: "Replace only unused induction variables and only when the cost is cheap".


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5601-5603
-// Check if there are any loop exit values which are only used once within the
-// loop which may potentially be optimized with a call to rewriteLoopExitValue.
-static bool LoopExitValHasSingleUse(Loop *L) {
----------------
I like deferring this to `rewriteLoopExitValue` itself. 👍


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1320
+            bool UsedInIndPhi = false;
+            for (auto &U : Inst->uses()) {
+              Phi = dyn_cast<PHINode>(U.getUser());
----------------
[style]


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1333
+          bool HasUseInLoop = false;
+          for (auto &U : Inst->uses()) {
+            Instruction *I = dyn_cast<Instruction>(U.getUser());
----------------
[style] No Almost-Always-Auto

[suggestion] Instead of set-flag-and-break, you may also consider `if (llvm::any_of(Inst->users(), [](User *U) {...})) continue;`. Alternatively, you could move all the conditions into a separate function where you just `return false;` when a violating condition is found no matter what loops it is nested in.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1340
+            BinaryOperator *B = dyn_cast<BinaryOperator>(I);
+            if (B && B != ID.getInductionBinOp()) {
+              HasUseInLoop = true;
----------------
This tries to check the "instruction is induction-phi" and "instruction is induction-operator" cases at the same time. You already determined which of the two cases it is above. That the wrong case applies here. `isInductionPHI` does not necessarily depend on there being an BinaryOperator as it works on the normalized SCEV. I.e. there might not be a `BinaryOperator` at all and the condition just pass through.

Could we merge the the two `for (auto &U : Inst->uses())` loops, determine which of the two cases it is, and then require exactly one `PHINode` or `BinaryOperator` use?


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1416
+         ReplaceExitValue == UnusedIndVar) &&
+        !LoopCanBeDel && Phi.HighCost)
       continue;
----------------
Is this assuming that unused induction variables are always cheap?


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

https://reviews.llvm.org/D125990