[PATCH] D109845: [SCEV] Correctly propagate nowrap flags across scopes when folding invariant add through addrec

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:2808
+        auto *SinglePred = AddRecLoop->getLoopPreheader();
+        if (!SinglePred || !SinglePred->getSingleSuccessor())
+          return false;
----------------
mkazantsev wrote:
> Preheader has single successor by definition.
Yep, good catch.  This is overly general from an earlier version of the code which had broader scope.  


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:2825
+          } else if (isa<SCEVConstant>(S)) {
+            return SinglePred == &SinglePred->getParent()->getEntryBlock();
+          }
----------------
mkazantsev wrote:
> Do we reject constants due to some block-related reasoning? Aren't they always available?
I don't follow your question?

We're analyzing a non-constant expression.  If we find a constant operand, we need to prove that *this occurrence* of the expression must execute on entry to the function.  Does that answer your concern?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:2831
+        return (FoundScope &&
+                isGuaranteedToTransferExecutionToSuccessor(SinglePred));
+      }();
----------------
mkazantsev wrote:
> Imagine the situation when inner loop is entered (and existing checks basically only test it), but always side-exits on 1st iteration in a call that is inside the inner loop. So actually whatever flags the inner AddRec has, it doesn't matter because they will never actually come in play. Will they still be propagated to outside?
I believe this is still fine.  Here's my reasoning:

The flags being present on the inner addrec disallows the case where the call is before all UB triggering uses of the inner addrec.  See usage of programUndefinedIfPoison in isSCEVExprNeverPoison.  

As such, we know that if we reach the inner loop, we must execute UB if the inner add-rec's flags produce poison.

We also know that the inner add (not addrec!) must also trigger UB on overflow.  (From argument)

Together, those let us know that the loop invariant add can have flags within the inner loop.  (This doesn't correspond to a SCEV, but is a useful mental stepping stone.)

All that's left is proving that all instances of the add in the defining scope must reach the one in the inner loop.  

Thus, we're good.  (Assuming that all flags were set correctly to start at least.)


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

https://reviews.llvm.org/D109845