[llvm] r323077 - [SCEV] Fix isLoopEntryGuardedByCond usage
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 22 10:30:15 PST 2018
Given this appears to be most every callsite of
isLoopEntryGuardedByCond, would it makes sense to just have the function
return false if the values aren't available?
Philip
On 01/21/2018 11:31 PM, Serguei Katkov via llvm-commits wrote:
> Author: skatkov
> Date: Sun Jan 21 23:31:41 2018
> New Revision: 323077
>
> URL: http://llvm.org/viewvc/llvm-project?rev=323077&view=rev
> Log:
> [SCEV] Fix isLoopEntryGuardedByCond usage
>
> ScalarEvolution::isKnownPredicate invokes isLoopEntryGuardedByCond without check
> that SCEV is available at entry point of the loop. It is incorrect and fixed by patch.
>
> Reviewers: sanjoy, mkazantsev, anna, dorit
> Reviewed By: mkazantsev
> Subscribers: llvm-commits
> Differential Revision: https://reviews.llvm.org/D42165
>
> Added:
> llvm/trunk/test/Transforms/IndVarSimplify/inner-loop.ll
> Modified:
> llvm/trunk/lib/Analysis/ScalarEvolution.cpp
> llvm/trunk/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
>
> Modified: llvm/trunk/lib/Analysis/ScalarEvolution.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ScalarEvolution.cpp?rev=323077&r1=323076&r2=323077&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Analysis/ScalarEvolution.cpp (original)
> +++ llvm/trunk/lib/Analysis/ScalarEvolution.cpp Sun Jan 21 23:31:41 2018
> @@ -8669,7 +8669,8 @@ bool ScalarEvolution::isKnownPredicate(I
> bool RightGuarded = false;
> if (LAR) {
> const Loop *L = LAR->getLoop();
> - if (isLoopEntryGuardedByCond(L, Pred, LAR->getStart(), RHS) &&
> + if (isAvailableAtLoopEntry(RHS, L) &&
> + isLoopEntryGuardedByCond(L, Pred, LAR->getStart(), RHS) &&
> isLoopBackedgeGuardedByCond(L, Pred, LAR->getPostIncExpr(*this), RHS)) {
> if (!RAR) return true;
> LeftGuarded = true;
> @@ -8677,7 +8678,8 @@ bool ScalarEvolution::isKnownPredicate(I
> }
> if (RAR) {
> const Loop *L = RAR->getLoop();
> - if (isLoopEntryGuardedByCond(L, Pred, LHS, RAR->getStart()) &&
> + if (isAvailableAtLoopEntry(LHS, L) &&
> + isLoopEntryGuardedByCond(L, Pred, LHS, RAR->getStart()) &&
> isLoopBackedgeGuardedByCond(L, Pred, LHS, RAR->getPostIncExpr(*this))) {
> if (!LAR) return true;
> RightGuarded = true;
> @@ -9058,6 +9060,12 @@ bool
> ScalarEvolution::isLoopEntryGuardedByCond(const Loop *L,
> ICmpInst::Predicate Pred,
> const SCEV *LHS, const SCEV *RHS) {
> + // Both LHS and RHS must be available at loop entry.
> + assert(isAvailableAtLoopEntry(LHS, L) &&
> + "LHS is not available at Loop Entry");
> + assert(isAvailableAtLoopEntry(RHS, L) &&
> + "RHS is not available at Loop Entry");
> +
> // Interpret a null as meaning no loop, where there is obviously no guard
> // (interprocedural conditions notwithstanding).
> if (!L) return false;
>
> Modified: llvm/trunk/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp?rev=323077&r1=323076&r2=323077&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp Sun Jan 21 23:31:41 2018
> @@ -934,9 +934,9 @@ LoopStructure::parseLoopStructure(Scalar
> return None;
> }
>
> - if (!SE.isLoopEntryGuardedByCond(
> - &L, BoundPred, IndVarStart,
> - SE.getAddExpr(RightSCEV, Step))) {
> + if (!SE.isAvailableAtLoopEntry(RightSCEV, &L) ||
> + !SE.isLoopEntryGuardedByCond(&L, BoundPred, IndVarStart,
> + SE.getAddExpr(RightSCEV, Step))) {
> FailureReason = "Induction variable start not bounded by upper limit";
> return None;
> }
> @@ -948,7 +948,8 @@ LoopStructure::parseLoopStructure(Scalar
> RightValue = B.CreateAdd(RightValue, One);
> }
> } else {
> - if (!SE.isLoopEntryGuardedByCond(&L, BoundPred, IndVarStart, RightSCEV)) {
> + if (!SE.isAvailableAtLoopEntry(RightSCEV, &L) ||
> + !SE.isLoopEntryGuardedByCond(&L, BoundPred, IndVarStart, RightSCEV)) {
> FailureReason = "Induction variable start not bounded by upper limit";
> return None;
> }
> @@ -1014,9 +1015,10 @@ LoopStructure::parseLoopStructure(Scalar
> return None;
> }
>
> - if (!SE.isLoopEntryGuardedByCond(
> - &L, BoundPred, IndVarStart,
> - SE.getMinusSCEV(RightSCEV, SE.getOne(RightSCEV->getType())))) {
> + if (!SE.isAvailableAtLoopEntry(RightSCEV, &L) ||
> + !SE.isLoopEntryGuardedByCond(
> + &L, BoundPred, IndVarStart,
> + SE.getMinusSCEV(RightSCEV, SE.getOne(RightSCEV->getType())))) {
> FailureReason = "Induction variable start not bounded by lower limit";
> return None;
> }
> @@ -1028,7 +1030,8 @@ LoopStructure::parseLoopStructure(Scalar
> RightValue = B.CreateSub(RightValue, One);
> }
> } else {
> - if (!SE.isLoopEntryGuardedByCond(&L, BoundPred, IndVarStart, RightSCEV)) {
> + if (!SE.isAvailableAtLoopEntry(RightSCEV, &L) ||
> + !SE.isLoopEntryGuardedByCond(&L, BoundPred, IndVarStart, RightSCEV)) {
> FailureReason = "Induction variable start not bounded by lower limit";
> return None;
> }
>
> Added: llvm/trunk/test/Transforms/IndVarSimplify/inner-loop.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/IndVarSimplify/inner-loop.ll?rev=323077&view=auto
> ==============================================================================
> --- llvm/trunk/test/Transforms/IndVarSimplify/inner-loop.ll (added)
> +++ llvm/trunk/test/Transforms/IndVarSimplify/inner-loop.ll Sun Jan 21 23:31:41 2018
> @@ -0,0 +1,54 @@
> +; RUN: opt < %s -indvars -S | FileCheck %s
> +
> +; This is regression test for the bug in ScalarEvolution::isKnownPredicate.
> +; It does not check whether SCEV is available at loop entry before invoking
> +; and utility function isLoopEntryGuardedByCond and that leads to miscompile.
> +
> +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128-ni:1"
> +target triple = "x86_64-unknown-linux-gnu"
> +
> +declare void @foo(i64)
> +declare void @bar(i32)
> +
> +define void @test(i8* %arr) {
> +entry:
> + br label %outer_header
> +
> +outer_header:
> + %i = phi i32 [40, %entry], [%i.next, %outer_latch]
> + %i.64 = sext i32 %i to i64
> + br label %inner_header
> +
> +inner_header:
> + %j = phi i32 [27, %outer_header], [%j.next, %inner_backedge]
> + %j1 = zext i32 %j to i64
> +; The next 4 lines are required for avoid widening of %j and
> +; SCEV at %cmp would not be AddRec.
> + %gep = getelementptr inbounds i8, i8* %arr, i64 %j1
> + %ld = load i8, i8* %gep
> + %ec = icmp eq i8 %ld, 0
> + br i1 %ec, label %return, label %inner_backedge
> +
> +inner_backedge:
> + %cmp = icmp ult i32 %j, %i
> + %s = select i1 %cmp, i32 %i, i32 %j
> +; Select should not be simplified because if
> +; %i == 26 and %j == 27, %s should be equal to %j.
> +; In case of a bug the instruction is simplified to
> +; %s = select i1 true, i32 %0, i32 %j
> +; CHECK-NOT: %s = select i1 true
> + call void @bar(i32 %s)
> + %j.next = add nsw i32 %j, -2
> + %cond = icmp ult i32 %j, 3
> + br i1 %cond, label %outer_latch, label %inner_header
> +
> +outer_latch:
> + %i.next = add i32 %i, -1
> + %cond2 = icmp sgt i32 %i.next, 13
> +; This line is just for forcing widening of %i
> + call void @foo(i64 %i.64)
> + br i1 %cond2, label %outer_header, label %return
> +
> +return:
> + ret void
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list