[PATCH] D62939: Strengthen LFTR's ability to replace IVs

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2106
 
+/// Return true undefined behavior would provable be executed on all iterations
+/// for which Root produced a poison result, and the loop exit condition for
----------------
nit: Return true //if// ... provably


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2109
+/// ExitingBB is evaluated.  Note that this doesn't say anything about whether
+/// LoopExiting BB is actually executed or taken.  This can be used to assess
+/// whether a new use of Root can be added before ExitingBB's terminator without
----------------
nit: s/LoopExisting BB/ExitingBB


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2128
+    Instruction *I = Worklist.pop_back_val();
+    if (!Visited.insert(I).second)
+      continue;
----------------
I feel like there's some confusion here between instructions that have a poison operand and instructions that produce poison. As you're inserting into Visited before doing the propagation test, Visited holds instructions that have a poison operand. However, in the `Visited.count(NotPoison)` check below you are assuming that Visited instructions return poison.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2143
+    if (isa<StoreInst>(PoisonI))
+      return true;
+    if (PoisonI == ExitingBB->getTerminator())
----------------
Why is this condition separate? This seems wrong for a) non-dominating store and b) poison being on the value operand, not the pointer operand.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2145
+    if (PoisonI == ExitingBB->getTerminator())
+      return true;
+    // If we know this must trigger UB on a path leading to our exit, we're
----------------
I think this isn't right as documented (branching on poison is not UB and there may be abnormal exits), though it seems okay for the purpose we need this.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62939