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

[Philip Reames via Phabricator via llvm-commits]

reames planned changes to this revision.
reames marked 2 inline comments as done.
reames added inline comments.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2143
+    if (isa<StoreInst>(PoisonI))
+      return true;
+    if (PoisonI == ExitingBB->getTerminator())
----------------
nikic wrote:
> 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.
I want to debate this point further, but for the moment, I'll simply remove the special store case.  As you point out, even without this it's really useful for pointer IVs, and we can separate that discussion into a separate patch/step.  


================
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:
> 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.
Gah, you're correct.  I'd forgotten the unswitching case.  

For the moment, I'll remove the branch handling from the current patch, and then come back to figure out how to handle it separately.  

That does mean we probably have another bug in LFTR though.  We treat a comparison used in the exit test as being concrete which per this discussion seems not guaranteed if the loop contains no side effects.  


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

https://reviews.llvm.org/D62939