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

[Philip Reames via Phabricator via llvm-commits]

reames marked 3 inline comments as done.
reames added a comment.

Address a few of Nikita's comment.  I ended up heavily revising the structure, so the line locations got a bit confusing.



================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2128
+    Instruction *I = Worklist.pop_back_val();
+    if (!Visited.insert(I).second)
+      continue;
----------------
nikic wrote:
> 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.
See the revised code.  It should be a lot more clear on this.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2143
+    if (isa<StoreInst>(PoisonI))
+      return true;
+    if (PoisonI == ExitingBB->getTerminator())
----------------
nikic wrote:
> 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.
That's specifically why it's separate.  The LangRef specifically gives an example of a poison value operand to a store being UB.  


================
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
----------------
nikic wrote:
> 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.
wait, wait.  No.  Branching on posion is *definitely* UB.  Or at least I believe it is!  What makes you think otherwise?


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

https://reviews.llvm.org/D62939