[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:2143
+    if (isa<StoreInst>(PoisonI))
+      return true;
+    if (PoisonI == ExitingBB->getTerminator())
----------------
reames wrote:
> 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.  
Storing poison is not UB for a normal store. Langref seems to say that it is UB for a volatile store, because volatile is externally observable. It only says that in an example though, and it's not clear how that follows from the preceding definitions.

The langref description of poison semantics is really quite bad. It describes the dependence behavior, but does not explain when exactly a dependence on poison triggers immediate 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
----------------
reames wrote:
> 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?
I used to think that as well, but unfortunately it's not the case. Branching on poison is not UB itself, you need a side-effect that is control dependent on the poisoned branch. There is a proposal to make branching on poison immediate UB (http://www.cs.utah.edu/~regehr/papers/undef-pldi17.pdf and https://lists.llvm.org/pipermail/llvm-dev/2016-October/106182.html), but I don't think this went anywhere.


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

https://reviews.llvm.org/D62939