[llvm] r271151 - [SCEV] Don't always add no-wrap flags to post-inc add recs
Sanjoy Das via llvm-commits
llvm-commits at lists.llvm.org
Sat May 28 18:25:43 PDT 2016
Hi Tobias, Johannes,
This change seems to have broken ScopInfo/wraping_signed_expr_1.ll .
I don't understand what is being tested there to confidently fix the
test, can one of you please take a look?
The change above fixes a long-standing issue in SCEV where it would
incorrectly mark an induction variable as no-overflow, so this fallout
isn't a 100% surprising. I haven't reverted the change yet since I've
already checked in some dependent changes (maybe wasn't the best idea
in hindsight) and would like to avoid unnecessary churn; but if things
will be easier for you if I reverted please let me know and I'll
revert.
As I understand, the difference made by r271151 is that
%indvars.iv.next is no longer "obviously" an <nsw> add recurrence.
But if you rotate the loop by -loop-rotate, SCEV should be able to
prove that the add rec for %indvars.iv.next is <nsw>; so maybe the
simplest fix is to just loop rotate both the @wrap and @nowrap
functions.
-- Sanjoy
On Sat, May 28, 2016 at 5:32 PM, Sanjoy Das via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
> Author: sanjoy
> Date: Sat May 28 19:32:17 2016
> New Revision: 271151
>
> URL: http://llvm.org/viewvc/llvm-project?rev=271151&view=rev
> Log:
> [SCEV] Don't always add no-wrap flags to post-inc add recs
>
> Fixes PR27315.
>
> The post-inc version of an add recurrence needs to "follow the same
> rules" as a normal add or subtract expression. Otherwise we miscompile
> programs like
>
> ```
> int main() {
> int a = 0;
> unsigned a_u = 0;
> volatile long last_value;
> do {
> a_u += 3;
> last_value = (long) ((int) a_u);
> if (will_add_overflow(a, 3)) {
> // Leave, and don't actually do the increment, so no UB.
> printf("last_value = %ld\n", last_value);
> exit(0);
> }
> a += 3;
> } while (a != 46);
> return 0;
> }
> ```
>
> This patch changes SCEV to put no-wrap flags on post-inc add recurrences
> only when the poison from a potential overflow will go ahead to cause
> undefined behavior.
>
> To avoid regressing performance too much, I've assumed infinite loops
> without side effects is undefined behavior to prove poison<->UB
> equivalence in more cases. This isn't ideal, but is not new to LLVM as
> a whole, and far better than the situation I'm trying to fix.
>
> Added:
> llvm/trunk/test/Analysis/ScalarEvolution/pr27315.ll
> Modified:
> llvm/trunk/include/llvm/Analysis/ScalarEvolution.h
> llvm/trunk/lib/Analysis/ScalarEvolution.cpp
> llvm/trunk/test/Analysis/ScalarEvolution/infer-prestart-no-wrap.ll
> llvm/trunk/test/Analysis/ScalarEvolution/nowrap-preinc-limits.ll
> llvm/trunk/test/Analysis/ScalarEvolution/nsw.ll
> llvm/trunk/test/Transforms/IndVarSimplify/elim-extend.ll
>
> Modified: llvm/trunk/include/llvm/Analysis/ScalarEvolution.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/ScalarEvolution.h?rev=271151&r1=271150&r2=271151&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Analysis/ScalarEvolution.h (original)
> +++ llvm/trunk/include/llvm/Analysis/ScalarEvolution.h Sat May 28 19:32:17 2016
> @@ -784,6 +784,10 @@ namespace llvm {
> SmallVector<PointerIntPair<const Loop *, 2, LoopDisposition>, 2>>
> LoopDispositions;
>
> + /// A cache of the predicate "does the given loop contain an instruction
> + /// that can throw?"
> + DenseMap<const Loop *, bool> LoopMayThrow;
> +
> /// Compute a LoopDisposition value.
> LoopDisposition computeLoopDisposition(const SCEV *S, const Loop *L);
>
> @@ -1124,6 +1128,12 @@ namespace llvm {
> /// it is not okay to annotate (+ a b) with <nsw> in the above example.
> bool isSCEVExprNeverPoison(const Instruction *I);
>
> + /// This is like \c isSCEVExprNeverPoison but it specifically works for
> + /// instructions that will get mapped to SCEV add recurrences. Return true
> + /// if \p I will never generate poison under the assumption that \p I is an
> + /// add recurrence on the loop \p L.
> + bool isAddRecNeverPoison(const Instruction *I, const Loop *L);
> +
> public:
> ScalarEvolution(Function &F, TargetLibraryInfo &TLI, AssumptionCache &AC,
> DominatorTree &DT, LoopInfo &LI);
>
> Modified: llvm/trunk/lib/Analysis/ScalarEvolution.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ScalarEvolution.cpp?rev=271151&r1=271150&r2=271151&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Analysis/ScalarEvolution.cpp (original)
> +++ llvm/trunk/lib/Analysis/ScalarEvolution.cpp Sat May 28 19:32:17 2016
> @@ -3980,8 +3980,6 @@ const SCEV *ScalarEvolution::createAddRe
> cast<SCEVAddRecExpr>(Accum)->getLoop() == L)) {
> SCEV::NoWrapFlags Flags = SCEV::FlagAnyWrap;
>
> - // If the increment doesn't overflow, then neither the addrec nor
> - // the post-increment will overflow.
> if (auto BO = MatchBinaryOp(BEValueV)) {
> if (BO->Opcode == Instruction::Add && BO->LHS == PN) {
> if (BO->IsNUW)
> @@ -4012,16 +4010,19 @@ const SCEV *ScalarEvolution::createAddRe
> const SCEV *StartVal = getSCEV(StartValueV);
> const SCEV *PHISCEV = getAddRecExpr(StartVal, Accum, L, Flags);
>
> - // Since the no-wrap flags are on the increment, they apply to the
> - // post-incremented value as well.
> - if (isLoopInvariant(Accum, L))
> - (void)getAddRecExpr(getAddExpr(StartVal, Accum), Accum, L, Flags);
> -
> // Okay, for the entire analysis of this edge we assumed the PHI
> // to be symbolic. We now need to go back and purge all of the
> // entries for the scalars that use the symbolic expression.
> forgetSymbolicName(PN, SymbolicName);
> ValueExprMap[SCEVCallbackVH(PN, this)] = PHISCEV;
> +
> + // We can add Flags to the post-inc expression only if we
> + // know that it us *undefined behavior* for BEValueV to
> + // overflow.
> + if (auto *BEInst = dyn_cast<Instruction>(BEValueV))
> + if (isLoopInvariant(Accum, L) && isAddRecNeverPoison(BEInst, L))
> + (void)getAddRecExpr(getAddExpr(StartVal, Accum), Accum, L, Flags);
> +
> return PHISCEV;
> }
> }
> @@ -4850,6 +4851,87 @@ bool ScalarEvolution::isSCEVExprNeverPoi
> return false;
> }
>
> +bool ScalarEvolution::isAddRecNeverPoison(const Instruction *I, const Loop *L) {
> + // If we know that \c I can never be poison period, then that's enough.
> + if (isSCEVExprNeverPoison(I))
> + return true;
> +
> + // For an add recurrence specifically, we assume that infinite loops without
> + // side effects are undefined behavior, and then reason as follows:
> + //
> + // If the add recurrence is poison in any iteration, it is poison on all
> + // future iterations (since incrementing poison yields poison). If the result
> + // of the add recurrence is fed into the loop latch condition and the loop
> + // does not contain any throws or exiting blocks other than the latch, we now
> + // have the ability to "choose" whether the backedge is taken or not (by
> + // choosing a sufficiently evil value for the poison feeding into the branch)
> + // for every iteration including and after the one in which \p I first became
> + // poison. There are two possibilities (let's call the iteration in which \p
> + // I first became poison as K):
> + //
> + // 1. In the set of iterations including and after K, the loop body executes
> + // no side effects. In this case executing the backege an infinte number
> + // of times will yield undefined behavior.
> + //
> + // 2. In the set of iterations including and after K, the loop body executes
> + // at least one side effect. In this case, that specific instance of side
> + // effect is control dependent on poison, which also yields undefined
> + // behavior.
> +
> + auto *ExitingBB = L->getExitingBlock();
> + auto *LatchBB = L->getLoopLatch();
> + if (!ExitingBB || !LatchBB || ExitingBB != LatchBB)
> + return false;
> +
> + SmallPtrSet<const Instruction *, 16> Pushed;
> + SmallVector<const Instruction *, 8> Stack;
> +
> + Pushed.insert(I);
> + for (auto *U : I->users())
> + if (Pushed.insert(cast<Instruction>(U)).second)
> + Stack.push_back(cast<Instruction>(U));
> +
> + bool LatchControlDependentOnPoison = false;
> + while (!Stack.empty()) {
> + const Instruction *I = Stack.pop_back_val();
> +
> + for (auto *U : I->users()) {
> + if (propagatesFullPoison(cast<Instruction>(U))) {
> + if (Pushed.insert(cast<Instruction>(U)).second)
> + Stack.push_back(cast<Instruction>(U));
> + } else if (auto *BI = dyn_cast<BranchInst>(U)) {
> + assert(BI->isConditional() && "Only possibility!");
> + if (BI->getParent() == LatchBB) {
> + LatchControlDependentOnPoison = true;
> + break;
> + }
> + }
> + }
> + }
> +
> + if (!LatchControlDependentOnPoison)
> + return false;
> +
> + // Now check if loop is no-throw, and cache the information. In the future,
> + // we can consider commoning this logic with LICMSafetyInfo into a separate
> + // analysis pass.
> +
> + auto Itr = LoopMayThrow.find(L);
> + if (Itr == LoopMayThrow.end()) {
> + bool MayThrow = false;
> + for (auto *BB : L->getBlocks()) {
> + MayThrow = any_of(*BB, [](Instruction &I) { return I.mayThrow(); });
> + if (MayThrow)
> + break;
> + }
> + auto InsertPair = LoopMayThrow.insert({L, MayThrow});
> + assert(InsertPair.second && "We just checked!");
> + Itr = InsertPair.first;
> + }
> +
> + return !Itr->second;
> +}
> +
> /// createSCEV - We know that there is no SCEV for the specified value. Analyze
> /// the expression.
> ///
> @@ -5439,6 +5521,8 @@ void ScalarEvolution::forgetLoop(const L
> // ValuesAtScopes map.
> for (Loop::iterator I = L->begin(), E = L->end(); I != E; ++I)
> forgetLoop(*I);
> +
> + LoopMayThrow.erase(L);
> }
>
> /// forgetValue - This method should be called by the client when it has
>
> Modified: llvm/trunk/test/Analysis/ScalarEvolution/infer-prestart-no-wrap.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/ScalarEvolution/infer-prestart-no-wrap.ll?rev=271151&r1=271150&r2=271151&view=diff
> ==============================================================================
> --- llvm/trunk/test/Analysis/ScalarEvolution/infer-prestart-no-wrap.ll (original)
> +++ llvm/trunk/test/Analysis/ScalarEvolution/infer-prestart-no-wrap.ll Sat May 28 19:32:17 2016
> @@ -1,6 +1,6 @@
> ; ; RUN: opt -analyze -scalar-evolution < %s | FileCheck %s
>
> -define void @infer.sext.0(i1* %c, i32 %start) {
> +define void @infer.sext.0(i1* %c, i32 %start, i32* %buf) {
> ; CHECK-LABEL: Classifying expressions for: @infer.sext.0
> entry:
> br label %loop
> @@ -12,6 +12,10 @@ define void @infer.sext.0(i1* %c, i32 %s
> %idx.inc.sext = sext i32 %idx.inc to i64
> ; CHECK: %idx.inc.sext = sext i32 %idx.inc to i64
> ; CHECK-NEXT: --> {(1 + (sext i32 %start to i64))<nsw>,+,1}<nsw><%loop>
> +
> + %buf.gep = getelementptr inbounds i32, i32* %buf, i32 %idx.inc
> + %val = load i32, i32* %buf.gep
> +
> %condition = icmp eq i32 %counter, 1
> %counter.inc = add i32 %counter, 1
> br i1 %condition, label %exit, label %loop
> @@ -20,7 +24,7 @@ define void @infer.sext.0(i1* %c, i32 %s
> ret void
> }
>
> -define void @infer.zext.0(i1* %c, i32 %start) {
> +define void @infer.zext.0(i1* %c, i32 %start, i32* %buf) {
> ; CHECK-LABEL: Classifying expressions for: @infer.zext.0
> entry:
> br label %loop
> @@ -32,6 +36,10 @@ define void @infer.zext.0(i1* %c, i32 %s
> %idx.inc.sext = zext i32 %idx.inc to i64
> ; CHECK: %idx.inc.sext = zext i32 %idx.inc to i64
> ; CHECK-NEXT: --> {(1 + (zext i32 %start to i64))<nuw><nsw>,+,1}<nuw><%loop>
> +
> + %buf.gep = getelementptr inbounds i32, i32* %buf, i32 %idx.inc
> + %val = load i32, i32* %buf.gep
> +
> %condition = icmp eq i32 %counter, 1
> %counter.inc = add i32 %counter, 1
> br i1 %condition, label %exit, label %loop
>
> Modified: llvm/trunk/test/Analysis/ScalarEvolution/nowrap-preinc-limits.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/ScalarEvolution/nowrap-preinc-limits.ll?rev=271151&r1=271150&r2=271151&view=diff
> ==============================================================================
> --- llvm/trunk/test/Analysis/ScalarEvolution/nowrap-preinc-limits.ll (original)
> +++ llvm/trunk/test/Analysis/ScalarEvolution/nowrap-preinc-limits.ll Sat May 28 19:32:17 2016
> @@ -36,7 +36,8 @@ define void @g(i1* %condition) {
> ; CHECK: %idx.inc2.sext = sext i32 %idx.inc2 to i64
> ; CHECK-NEXT: --> {2,+,3}<nuw><nsw><%loop>
>
> - %c = load volatile i1, i1* %condition
> + %cond.gep = getelementptr inbounds i1, i1* %condition, i32 %idx.inc
> + %c = load volatile i1, i1* %cond.gep
> br i1 %c, label %loop, label %exit
>
> exit:
>
> Modified: llvm/trunk/test/Analysis/ScalarEvolution/nsw.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/ScalarEvolution/nsw.ll?rev=271151&r1=271150&r2=271151&view=diff
> ==============================================================================
> --- llvm/trunk/test/Analysis/ScalarEvolution/nsw.ll (original)
> +++ llvm/trunk/test/Analysis/ScalarEvolution/nsw.ll Sat May 28 19:32:17 2016
> @@ -30,15 +30,17 @@ bb: ; preds = %bb1, %bb.nph
> %tmp8 = add nsw i32 %i.01, 1 ; <i32> [#uses=2]
> ; CHECK: %tmp8
> ; CHECK-NEXT: --> {1,+,1}<nuw><nsw><%bb>
> + %p.gep = getelementptr double, double* %p, i32 %tmp8
> + %p.val = load double, double* %p.gep
> br label %bb1
>
> bb1: ; preds = %bb
> %phitmp = sext i32 %tmp8 to i64 ; <i64> [#uses=1]
> ; CHECK: %phitmp
> ; CHECK-NEXT: --> {1,+,1}<nuw><nsw><%bb>
> - %tmp9 = getelementptr double, double* %p, i64 %phitmp ; <double*> [#uses=1]
> + %tmp9 = getelementptr inbounds double, double* %p, i64 %phitmp ; <double*> [#uses=1]
> ; CHECK: %tmp9
> -; CHECK-NEXT: --> {(8 + %p),+,8}<%bb>
> +; CHECK-NEXT: --> {(8 + %p)<nsw>,+,8}<nsw><%bb>
> %tmp10 = load double, double* %tmp9, align 8 ; <double> [#uses=1]
> %tmp11 = fcmp ogt double %tmp10, 2.000000e+00 ; <i1> [#uses=1]
> br i1 %tmp11, label %bb, label %bb1.return_crit_edge
> @@ -143,15 +145,15 @@ bb7:
> }
>
> ; CHECK-LABEL: PR12376
> -; CHECK: --> {(4 + %arg)<nsw>,+,4}<nuw><%bb2>{{ U: [^ ]+ S: [^ ]+}}{{ *}}Exits: (4 + (4 * ((3 + (-1 * %arg) + (%arg umax %arg1)) /u 4)) + %arg)
> +; CHECK: --> {(4 + %arg)<nsw>,+,4}<nuw><%bb2>{{ U: [^ ]+ S: [^ ]+}}{{ *}}Exits: (4 + (4 * ((-1 + (-1 * %arg) + ((4 + %arg)<nsw> umax %arg1)) /u 4)) + %arg)
> define void @PR12376(i32* nocapture %arg, i32* nocapture %arg1) {
> bb:
> br label %bb2
>
> bb2: ; preds = %bb2, %bb
> %tmp = phi i32* [ %arg, %bb ], [ %tmp4, %bb2 ]
> - %tmp3 = icmp ult i32* %tmp, %arg1
> %tmp4 = getelementptr inbounds i32, i32* %tmp, i64 1
> + %tmp3 = icmp ult i32* %tmp4, %arg1
> br i1 %tmp3, label %bb2, label %bb5
>
> bb5: ; preds = %bb2
> @@ -161,8 +163,8 @@ bb5:
> declare void @f(i32)
>
> ; CHECK-LABEL: nswnowrap
> -; CHECK: --> {(1 + %v),+,1}<nsw><%for.body>{{ U: [^ ]+ S: [^ ]+}}{{ *}}Exits: (2 + %v)
> -define void @nswnowrap(i32 %v) {
> +; CHECK: --> {(1 + %v)<nsw>,+,1}<nsw><%for.body>{{ U: [^ ]+ S: [^ ]+}}{{ *}}Exits: (2 + %v)
> +define void @nswnowrap(i32 %v, i32* %buf) {
> entry:
> %add = add nsw i32 %v, 1
> br label %for.body
> @@ -170,8 +172,10 @@ entry:
> for.body:
> %i.04 = phi i32 [ %v, %entry ], [ %inc, %for.body ]
> %inc = add nsw i32 %i.04, 1
> - tail call void @f(i32 %i.04)
> + %buf.gep = getelementptr inbounds i32, i32* %buf, i32 %inc
> + %buf.val = load i32, i32* %buf.gep
> %cmp = icmp slt i32 %i.04, %add
> + tail call void @f(i32 %i.04)
> br i1 %cmp, label %for.body, label %for.end
>
> for.end:
>
> Added: llvm/trunk/test/Analysis/ScalarEvolution/pr27315.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/ScalarEvolution/pr27315.ll?rev=271151&view=auto
> ==============================================================================
> --- llvm/trunk/test/Analysis/ScalarEvolution/pr27315.ll (added)
> +++ llvm/trunk/test/Analysis/ScalarEvolution/pr27315.ll Sat May 28 19:32:17 2016
> @@ -0,0 +1,31 @@
> +; RUN: opt -analyze -scalar-evolution < %s | FileCheck %s
> +
> +declare i1 @use(i64)
> +
> +define void @f_0() {
> +; CHECK-LABEL: Classifying expressions for: @f_0
> +
> +; CHECK: %iv = phi i32 [ 0, %entry ], [ %iv.inc.nowrap, %be ]
> +; CHECK-NEXT: --> {0,+,1}<nuw><nsw><%loop>
> +; CHECK: %iv.inc.maywrap = add i32 %iv, 1
> +; CHECK-NEXT: --> {1,+,1}<nuw><%loop>
> +; CHECK: %iv.inc.maywrap.sext = sext i32 %iv.inc.maywrap to i64
> +; CHECK-NEXT: --> (sext i32 {1,+,1}<nuw><%loop> to i64)
> +entry:
> + br label %loop
> +
> +loop:
> + %iv = phi i32 [ 0, %entry ], [ %iv.inc.nowrap, %be ]
> + %iv.inc.maywrap = add i32 %iv, 1
> + %iv.inc.maywrap.sext = sext i32 %iv.inc.maywrap to i64
> + %cond0 = call i1 @use(i64 %iv.inc.maywrap.sext)
> + br i1 %cond0, label %be, label %leave
> +
> +be:
> + %iv.inc.nowrap = add nsw i32 %iv, 1
> + %be.cond = call i1 @use(i64 0) ;; Get an unanalyzable value
> + br i1 %be.cond, label %loop, label %leave
> +
> +leave:
> + ret void
> +}
>
> Modified: llvm/trunk/test/Transforms/IndVarSimplify/elim-extend.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/IndVarSimplify/elim-extend.ll?rev=271151&r1=271150&r2=271151&view=diff
> ==============================================================================
> --- llvm/trunk/test/Transforms/IndVarSimplify/elim-extend.ll (original)
> +++ llvm/trunk/test/Transforms/IndVarSimplify/elim-extend.ll Sat May 28 19:32:17 2016
> @@ -22,7 +22,7 @@ loop:
> store i8 0, i8* %postadr
> %postivnsw = add nsw i32 %ivnsw, 1
> %postofsnsw = sext i32 %postivnsw to i64
> - %postadrnsw = getelementptr i8, i8* %base, i64 %postofsnsw
> + %postadrnsw = getelementptr inbounds i8, i8* %base, i64 %postofsnsw
> store i8 0, i8* %postadrnsw
> %cond = icmp sgt i32 %limit, %iv
> br i1 %cond, label %loop, label %exit
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
--
Sanjoy Das
http://playingwithpointers.com
More information about the llvm-commits
mailing list