[PATCH] IndVarSimplify: Don't let LFTR compare against a poison value

[David Majnemer]

On Wed, Sep 3, 2014 at 7:33 PM, Andrew Trick <atrick at apple.com> wrote:

> I'll say LGTM because fixing a bug is an improvement. This is unfortunate
> though because LFTR does not care about overflow and we don't want to
> pessimize code just because SCEV was able to infer NSW. The problem is that
> we can't express that NSW applies to some uses an not others.
>
> The fix is also not totally robust because SCEV could, in theory, drop NSW
> flags even though they exist in IR. The induction variable was not
> necessarily produced by SCEVExpander. I think making it totally robust
> would involve getting a new expression for CmpIndVar by applying non-NSW
> AddExpr's (similar to genLoopLimit). That is of course much more
> complexity, hence risk, so maybe best as a FIXME.
>

genLoopLimit assumes that the IndVar has a SCEV, when could the induction
variable not be produced by SCEVExpander?

I quickly hacked the following together, it seems to restore the old
behavior in at least one case.  It tries to see if signed or unsigned
overflow can occur by looking at the initial value and the number of
backedges.

diff --git a/lib/Transforms/Scalar/IndVarSimplify.cpp
b/lib/Transforms/Scalar/IndVarSimplify.cpp
index 27e5ec8..ccc906d 100644
--- a/lib/Transforms/Scalar/IndVarSimplify.cpp
+++ b/lib/Transforms/Scalar/IndVarSimplify.cpp
@@ -1655,8 +1655,31 @@ LinearFunctionTestReplace(Loop *L,
     // FIXME: In theory, SCEV could drop flags even though they exist in
IR.
     // A more robust solution would involve getting a new expression for
     // CmpIndVar by applying non-NSW/NUW AddExprs.
+    auto WrappingFlags =
+        ScalarEvolution::setFlags(SCEV::FlagNUW, SCEV::FlagNSW);
+    const SCEV *IVInit = IncrementedIndvarSCEV->getStart();
+    if (SE->getTypeSizeInBits(IVInit->getType()) >
+        SE->getTypeSizeInBits(IVCount->getType()))
+      IVInit = SE->getTruncateExpr(IVInit, IVCount->getType());
+    if (const auto *IVInitCst = dyn_cast<SCEVConstant>(IVInit)) {
+      if (const auto *BackedgeTakenCountCst =
+              dyn_cast<SCEVConstant>(BackedgeTakenCount)) {
+        const APInt &IVInitVal = IVInitCst->getValue()->getValue();
+        APInt BackedgeTakenCountVal =
+            BackedgeTakenCountCst->getValue()->getValue();
+        bool SignedOverflow, UnsignedOverflow;
+        IVInitVal.uadd_ov(BackedgeTakenCountVal, UnsignedOverflow);
+        IVInitVal.sadd_ov(BackedgeTakenCountVal, SignedOverflow);
+        if (!UnsignedOverflow)
+          WrappingFlags =
+              ScalarEvolution::clearFlags(WrappingFlags, SCEV::FlagNUW);
+        if (!SignedOverflow)
+          WrappingFlags =
+              ScalarEvolution::clearFlags(WrappingFlags, SCEV::FlagNSW);
+      }
+    }
     if
(!ScalarEvolution::maskFlags(IncrementedIndvarSCEV->getNoWrapFlags(),
-                                    SCEV::FlagNUW | SCEV::FlagNSW)) {
+                                    WrappingFlags)) {
       // Add one to the "backedge-taken" count to get the trip count.
       // This addition may overflow, which is valid as long as the
comparison is
       // truncated to BackedgeTakenCount->getType().

Does it seem like a reasonable approach?


> Out of curiosity, can you explain how we came to generate bad code for
> this case (so I don't have to debug the test case)? Was there an
> optimization after LFTR that assumed NSW at the compare? Do we end up
> widening the loop test and failing to exit?
>
> FWIW: The check against FlagAnyWrap was a little confusing to me, as
> opposed to simply:
> if (maskFlags(Expr->getNoWrapFlags(), SCEV::NSW | SCEV::NUW))
>
> REPOSITORY
>   rL LLVM
>
> http://reviews.llvm.org/D5174
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140905/d8809af7/attachment.html>