[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:2111
+static bool MustTriggerUB(Instruction *PoisonI,
+                          const SmallSet<Instruction *, 16>& Visited,
+                          DominatorTree *DT) {
----------------
Rename `PoisonI` to `I` and `Visited` to `KnownPoison`? `PoisonI` is a bit odd in that it needn't be itself poison. Can also drop `DT` argument.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2131
+      if (auto *OpI = dyn_cast<Instruction>(OpV))
+        if (KnownPoison.count(OpI) && DT->dominates(OpI, I))
+          return true;
----------------
I don't understand the phi logic here. If you have
```
x1 = poison
if (...)
  x2 = not poison
x3 = phi(x1, x2)
```
then x3 is not guaranteed poison, even though x1 dominates the phi.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2162
+    if (DT->dominates(I, OnPathTo) &&
+        MustTriggerUB(I, KnownPoison, DT))
+      return true;
----------------
Should probably swap these conditions.


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

https://reviews.llvm.org/D62939