[llvm] r289453 - Revert "[SCEVExpand] do not hoist divisions by zero (PR30935)"
Reid Kleckner via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 12 10:52:32 PST 2016
Author: rnk
Date: Mon Dec 12 12:52:32 2016
New Revision: 289453
URL: http://llvm.org/viewvc/llvm-project?rev=289453&view=rev
Log:
Revert "[SCEVExpand] do not hoist divisions by zero (PR30935)"
Reverts r289412. It caused an OOB PHI operand access in instcombine when
ASan is enabled. Reduction in progress.
Also reverts "[SCEVExpander] Add a test case related to r289412"
Removed:
llvm/trunk/test/Transforms/LoopIdiom/pr30935.ll
Modified:
llvm/trunk/lib/Analysis/ScalarEvolution.cpp
llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp
llvm/trunk/unittests/Analysis/ScalarEvolutionTest.cpp
Modified: llvm/trunk/lib/Analysis/ScalarEvolution.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ScalarEvolution.cpp?rev=289453&r1=289452&r2=289453&view=diff
==============================================================================
--- llvm/trunk/lib/Analysis/ScalarEvolution.cpp (original)
+++ llvm/trunk/lib/Analysis/ScalarEvolution.cpp Mon Dec 12 12:52:32 2016
@@ -2130,7 +2130,7 @@ const SCEV *ScalarEvolution::getAddExpr(
}
// Okay, check to see if the same value occurs in the operand list more than
- // once. If so, merge them together into a multiply expression. Since we
+ // once. If so, merge them together into an multiply expression. Since we
// sorted the list, these values are required to be adjacent.
Type *Ty = Ops[0]->getType();
bool FoundMatch = false;
Modified: llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp?rev=289453&r1=289452&r2=289453&view=diff
==============================================================================
--- llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp (original)
+++ llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp Mon Dec 12 12:52:32 2016
@@ -166,16 +166,6 @@ Value *SCEVExpander::InsertNoopCastOfTo(
return ReuseOrCreateCast(I, Ty, Op, IP);
}
-// Return true when S may contain the value zero.
-static bool mayBeValueZero(Value *V) {
- if (ConstantInt *C = dyn_cast<ConstantInt>(V))
- if (!C->isZero())
- return false;
-
- // All other expressions may have a zero value.
- return true;
-}
-
/// InsertBinop - Insert the specified binary operator, doing a small amount
/// of work to avoid inserting an obviously redundant operation.
Value *SCEVExpander::InsertBinop(Instruction::BinaryOps Opcode,
@@ -208,17 +198,14 @@ Value *SCEVExpander::InsertBinop(Instruc
DebugLoc Loc = Builder.GetInsertPoint()->getDebugLoc();
SCEVInsertPointGuard Guard(Builder, this);
- // Only move the insertion point up when it is not a division by zero.
- if (Opcode != Instruction::UDiv || !mayBeValueZero(RHS)) {
- // Move the insertion point out of as many loops as we can.
- while (const Loop *L = SE.LI.getLoopFor(Builder.GetInsertBlock())) {
- if (!L->isLoopInvariant(LHS) || !L->isLoopInvariant(RHS)) break;
- BasicBlock *Preheader = L->getLoopPreheader();
- if (!Preheader) break;
+ // Move the insertion point out of as many loops as we can.
+ while (const Loop *L = SE.LI.getLoopFor(Builder.GetInsertBlock())) {
+ if (!L->isLoopInvariant(LHS) || !L->isLoopInvariant(RHS)) break;
+ BasicBlock *Preheader = L->getLoopPreheader();
+ if (!Preheader) break;
- // Ok, move up a level.
- Builder.SetInsertPoint(Preheader->getTerminator());
- }
+ // Ok, move up a level.
+ Builder.SetInsertPoint(Preheader->getTerminator());
}
// If we haven't found this binop, insert it.
@@ -1679,46 +1666,31 @@ Value *SCEVExpander::expand(const SCEV *
// Compute an insertion point for this SCEV object. Hoist the instructions
// as far out in the loop nest as possible.
Instruction *InsertPt = &*Builder.GetInsertPoint();
- bool SafeToHoist = !SCEVExprContains(S, [](const SCEV *S) {
- if (const auto *D = dyn_cast<SCEVUDivExpr>(S)) {
- if (const auto *SC = dyn_cast<SCEVConstant>(D->getRHS()))
- // Division by non-zero constants can be hoisted.
- return SC->getValue()->isZero();
-
- // All other divisions should not be moved as they may be divisions by
- // zero and should be kept within the conditions of the surrounding
- // loops that guard their execution (see PR30935.)
- return true;
+ for (Loop *L = SE.LI.getLoopFor(Builder.GetInsertBlock());;
+ L = L->getParentLoop())
+ if (SE.isLoopInvariant(S, L)) {
+ if (!L) break;
+ if (BasicBlock *Preheader = L->getLoopPreheader())
+ InsertPt = Preheader->getTerminator();
+ else {
+ // LSR sets the insertion point for AddRec start/step values to the
+ // block start to simplify value reuse, even though it's an invalid
+ // position. SCEVExpander must correct for this in all cases.
+ InsertPt = &*L->getHeader()->getFirstInsertionPt();
}
- return false;
- });
- if (SafeToHoist) {
- for (Loop *L = SE.LI.getLoopFor(Builder.GetInsertBlock());;
- L = L->getParentLoop())
- if (SE.isLoopInvariant(S, L)) {
- if (!L) break;
- if (BasicBlock *Preheader = L->getLoopPreheader())
- InsertPt = Preheader->getTerminator();
- else {
- // LSR sets the insertion point for AddRec start/step values to the
- // block start to simplify value reuse, even though it's an invalid
- // position. SCEVExpander must correct for this in all cases.
- InsertPt = &*L->getHeader()->getFirstInsertionPt();
- }
- } else {
- // If the SCEV is computable at this level, insert it into the header
- // after the PHIs (and after any other instructions that we've inserted
- // there) so that it is guaranteed to dominate any user inside the loop.
- if (L && SE.hasComputableLoopEvolution(S, L) && !PostIncLoops.count(L))
- InsertPt = &*L->getHeader()->getFirstInsertionPt();
- while (InsertPt->getIterator() != Builder.GetInsertPoint() &&
- (isInsertedInstruction(InsertPt) ||
- isa<DbgInfoIntrinsic>(InsertPt))) {
- InsertPt = &*std::next(InsertPt->getIterator());
- }
- break;
+ } else {
+ // If the SCEV is computable at this level, insert it into the header
+ // after the PHIs (and after any other instructions that we've inserted
+ // there) so that it is guaranteed to dominate any user inside the loop.
+ if (L && SE.hasComputableLoopEvolution(S, L) && !PostIncLoops.count(L))
+ InsertPt = &*L->getHeader()->getFirstInsertionPt();
+ while (InsertPt->getIterator() != Builder.GetInsertPoint() &&
+ (isInsertedInstruction(InsertPt) ||
+ isa<DbgInfoIntrinsic>(InsertPt))) {
+ InsertPt = &*std::next(InsertPt->getIterator());
}
- }
+ break;
+ }
// Check to see if we already expanded this here.
auto I = InsertedExpressions.find(std::make_pair(S, InsertPt));
Removed: llvm/trunk/test/Transforms/LoopIdiom/pr30935.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/LoopIdiom/pr30935.ll?rev=289452&view=auto
==============================================================================
--- llvm/trunk/test/Transforms/LoopIdiom/pr30935.ll (original)
+++ llvm/trunk/test/Transforms/LoopIdiom/pr30935.ll (removed)
@@ -1,94 +0,0 @@
-; RUN: opt -loop-idiom -S < %s | FileCheck %s
-
-; CHECK-LABEL: define i32 @main(
-; CHECK: udiv
-; CHECK-NOT: udiv
-; CHECK: call void @llvm.memset.p0i8.i64
-
- at a = common local_unnamed_addr global [4 x i8] zeroinitializer, align 1
- at b = common local_unnamed_addr global i32 0, align 4
- at c = common local_unnamed_addr global i32 0, align 4
- at d = common local_unnamed_addr global i32 0, align 4
- at e = common local_unnamed_addr global i32 0, align 4
- at f = common local_unnamed_addr global i32 0, align 4
- at g = common local_unnamed_addr global i32 0, align 4
- at h = common local_unnamed_addr global i64 0, align 8
-
-
-define i32 @main() local_unnamed_addr #0 {
-entry:
- %0 = load i32, i32* @e, align 4
- %tobool19 = icmp eq i32 %0, 0
- %1 = load i32, i32* @c, align 4
- %cmp10 = icmp eq i32 %1, 0
- %2 = load i32, i32* @g, align 4
- %3 = load i32, i32* @b, align 4
- %tobool = icmp eq i32 %0, 0
- br label %for.cond
-
-for.cond.loopexit: ; preds = %for.inc14
- br label %for.cond.backedge
-
-for.cond: ; preds = %for.cond.backedge, %entry
- %.pr = load i32, i32* @f, align 4
- %cmp20 = icmp eq i32 %.pr, 0
- br i1 %cmp20, label %for.cond2.preheader.preheader, label %for.cond.backedge
-
-for.cond.backedge: ; preds = %for.cond, %for.cond.loopexit
- br label %for.cond
-
-for.cond2.preheader.preheader: ; preds = %for.cond
- br label %for.cond2.preheader
-
-for.cond2.preheader: ; preds = %for.cond2.preheader.preheader, %for.inc14
- br i1 %tobool19, label %for.cond9, label %for.body3.lr.ph
-
-for.body3.lr.ph: ; preds = %for.cond2.preheader
- %div = udiv i32 %2, %3
- %conv = zext i32 %div to i64
- br label %for.body3
-
-for.cond4.for.cond2.loopexit_crit_edge: ; preds = %for.body6
- store i32 0, i32* @d, align 4
- br label %for.cond2.loopexit
-
-for.cond2.loopexit: ; preds = %for.cond4.for.cond2.loopexit_crit_edge, %for.body3
- br i1 %tobool, label %for.cond2.for.cond9_crit_edge, label %for.body3
-
-for.body3: ; preds = %for.body3.lr.ph, %for.cond2.loopexit
- %.pr17 = load i32, i32* @d, align 4
- %tobool518 = icmp eq i32 %.pr17, 0
- br i1 %tobool518, label %for.cond2.loopexit, label %for.body6.preheader
-
-for.body6.preheader: ; preds = %for.body3
- %4 = zext i32 %.pr17 to i64
- br label %for.body6
-
-for.body6: ; preds = %for.body6.preheader, %for.body6
- %indvars.iv = phi i64 [ %4, %for.body6.preheader ], [ %indvars.iv.next, %for.body6 ]
- %add = add nuw nsw i64 %conv, %indvars.iv
- %arrayidx = getelementptr inbounds [4 x i8], [4 x i8]* @a, i64 0, i64 %add
- store i8 1, i8* %arrayidx, align 1
- %5 = trunc i64 %indvars.iv to i32
- %inc = add i32 %5, 1
- %tobool5 = icmp eq i32 %inc, 0
- %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
- br i1 %tobool5, label %for.cond4.for.cond2.loopexit_crit_edge, label %for.body6
-
-for.cond2.for.cond9_crit_edge: ; preds = %for.cond2.loopexit
- store i64 %conv, i64* @h, align 8
- br label %for.cond9
-
-for.cond9: ; preds = %for.cond2.for.cond9_crit_edge, %for.cond2.preheader
- br i1 %cmp10, label %for.body12, label %for.inc14
-
-for.body12: ; preds = %for.cond9
- ret i32 0
-
-for.inc14: ; preds = %for.cond9
- %6 = load i32, i32* @f, align 4
- %inc15 = add i32 %6, 1
- store i32 %inc15, i32* @f, align 4
- %cmp = icmp eq i32 %inc15, 0
- br i1 %cmp, label %for.cond2.preheader, label %for.cond.loopexit
-}
Modified: llvm/trunk/unittests/Analysis/ScalarEvolutionTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Analysis/ScalarEvolutionTest.cpp?rev=289453&r1=289452&r2=289453&view=diff
==============================================================================
--- llvm/trunk/unittests/Analysis/ScalarEvolutionTest.cpp (original)
+++ llvm/trunk/unittests/Analysis/ScalarEvolutionTest.cpp Mon Dec 12 12:52:32 2016
@@ -51,21 +51,13 @@ protected:
return ScalarEvolution(F, TLI, *AC, *DT, *LI);
}
- void runSCEVTest(Module &M, StringRef FuncName,
- function_ref<void(Function &F, DominatorTree &DT,
- LoopInfo &LI, ScalarEvolution &SE)>
- Test) {
- auto *F = M.getFunction(FuncName);
- ASSERT_NE(F, nullptr) << "Could not find " << FuncName;
- ScalarEvolution SE = buildSE(*F);
- Test(*F, *DT, *LI, SE);
- }
-
void runWithFunctionAndSE(
Module &M, StringRef FuncName,
function_ref<void(Function &F, ScalarEvolution &SE)> Test) {
- runSCEVTest(M, FuncName, [&](Function &F, DominatorTree &DT, LoopInfo &LI,
- ScalarEvolution &SE) { Test(F, SE); });
+ auto *F = M.getFunction(FuncName);
+ ASSERT_NE(F, nullptr) << "Could not find " << FuncName;
+ ScalarEvolution SE = buildSE(*F);
+ Test(*F, SE);
}
};
@@ -357,13 +349,6 @@ static Instruction *getInstructionByName
llvm_unreachable("Expected to find instruction!");
}
-static Argument *getArgByName(Function &F, StringRef Name) {
- for (auto &A : F.args())
- if (A.getName() == Name)
- return &A;
- llvm_unreachable("Expected to find argument!");
-}
-
TEST_F(ScalarEvolutionsTest, CommutativeExprOperandOrder) {
LLVMContext C;
SMDiagnostic Err;
@@ -547,94 +532,5 @@ TEST_F(ScalarEvolutionsTest, SCEVCompare
EXPECT_NE(nullptr, SE.getSCEV(Acc[0]));
}
-TEST_F(ScalarEvolutionsTest, BadHoistingSCEVExpander_PR30942) {
- LLVMContext C;
- SMDiagnostic Err;
- std::unique_ptr<Module> M = parseAssemblyString(
- "target datalayout = \"e-m:e-p:32:32-f64:32:64-f80:32-n8:16:32-S128\" "
- " "
- "define void @f_1(i32 %x, i32 %y, i32 %n, i1* %cond_buf) "
- " local_unnamed_addr { "
- "entry: "
- " %entrycond = icmp sgt i32 %n, 0 "
- " br i1 %entrycond, label %loop.ph, label %for.end "
- " "
- "loop.ph: "
- " br label %loop "
- " "
- "loop: "
- " %iv1 = phi i32 [ %iv1.inc, %right ], [ 0, %loop.ph ] "
- " %iv1.inc = add nuw nsw i32 %iv1, 1 "
- " %cond = load volatile i1, i1* %cond_buf "
- " br i1 %cond, label %left, label %right "
- " "
- "left: "
- " %div = udiv i32 %x, %y "
- " br label %right "
- " "
- "right: "
- " %exitcond = icmp eq i32 %iv1.inc, %n "
- " br i1 %exitcond, label %for.end.loopexit, label %loop "
- " "
- "for.end.loopexit: "
- " br label %for.end "
- " "
- "for.end: "
- " ret void "
- "} ",
- Err, C);
-
- assert(M && "Could not parse module?");
- assert(!verifyModule(*M) && "Must have been well formed!");
-
- runSCEVTest(*M, "f_1", [&](Function &F, DominatorTree &DT, LoopInfo &LI,
- ScalarEvolution &SE) {
- SCEVExpander Expander(SE, M->getDataLayout(), "unittests");
- auto *DivInst = getInstructionByName(F, "div");
-
- {
- auto *DivSCEV = SE.getSCEV(DivInst);
- auto *DivExpansion = Expander.expandCodeFor(
- DivSCEV, DivSCEV->getType(), DivInst->getParent()->getTerminator());
- auto *DivExpansionInst = dyn_cast<Instruction>(DivExpansion);
- ASSERT_NE(DivExpansionInst, nullptr);
- EXPECT_EQ(DivInst->getParent(), DivExpansionInst->getParent());
- }
-
- {
- auto *ArgY = getArgByName(F, "y");
- auto *DivFromScratchSCEV =
- SE.getUDivExpr(SE.getOne(ArgY->getType()), SE.getSCEV(ArgY));
-
- auto *DivFromScratchExpansion = Expander.expandCodeFor(
- DivFromScratchSCEV, DivFromScratchSCEV->getType(),
- DivInst->getParent()->getTerminator());
- auto *DivFromScratchExpansionInst =
- dyn_cast<Instruction>(DivFromScratchExpansion);
- ASSERT_NE(DivFromScratchExpansionInst, nullptr);
- EXPECT_EQ(DivInst->getParent(), DivFromScratchExpansionInst->getParent());
- }
-
- {
- auto *ArgY = getArgByName(F, "y");
- auto *One = SE.getOne(ArgY->getType());
- auto *DivFromScratchSCEV = SE.getUDivExpr(One, SE.getSCEV(ArgY));
- auto *L = LI.getLoopFor(DivInst->getParent());
- auto *ARFromScratchSCEV =
- SE.getAddRecExpr(DivFromScratchSCEV, One, L, SCEV::FlagAnyWrap);
-
- Expander.disableCanonicalMode();
-
- auto *ARFromScratchExpansion = Expander.expandCodeFor(
- ARFromScratchSCEV, ARFromScratchSCEV->getType(),
- DivInst->getParent()->getTerminator());
- auto *ARFromScratchExpansionInst =
- dyn_cast<Instruction>(ARFromScratchExpansion);
- ASSERT_NE(ARFromScratchExpansionInst, nullptr);
- ASSERT_FALSE(verifyFunction(F));
- }
- });
-}
-
} // end anonymous namespace
} // end namespace llvm
More information about the llvm-commits
mailing list