[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:2098
+/// actually poison.  This can be used to assess whether a new use of Root can
+/// be added at a location which is control equivalent wit OnPathTo (such as
+/// immediately before it) without introducing UB which didn't previously
----------------
wit -> with


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2106
+  // through all users we can easily track, and then check whether any of those
+  // users are provable UB and must execute before out exiting block might
+  // exit.
----------------
out -> our, though given the generalized formulation used here now, we shouldn't mention "exit block" anymore.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2265
+    if (!mustExecuteUBIfPoisonOnPathTo(Phi, ExitTerm, DT) &&
+        !hasConcreteDef(Phi))
+      continue;
----------------
Unfortunately, I don't think this is right :( The problem is that hasConcreteDef() deals with a similar but distinct issue: an undef IV, not a poison one.

Consider an IV of the form `{undef,+,1}` (without nuw/nsw, so never poison). In theory there is nothing wrong with using such an IV. However, due to LLVMs broken implementation of undef, it is generally illegal to add a new use to a value that is potentially undef, because the fact that two uses of undef must be the same will not be preserved. This is why the hasConcreteDef() check is necessary.

Now, the mustExecuteUBIfPoisonOnPathTo() check is correct if we assume that Phi is poison, but not if it is undef, because they have different propagation semantics. For example:

    %iv = {undef,+,1}
    %or = or %iv, 1
    %div = udiv -1, %or
    br -> exit

Here, if `%iv` were poison, then the `udiv` would trigger UB and it would be safe to add a new use. However, it is only undef here, in which case the `udiv` will *not* trigger UB. But because the check is performed under the assumption of poison, we'll incorrectly switch to this IV.


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

https://reviews.llvm.org/D62939