<div dir="ltr">Heads up, this caused a crash in instcombine during an ASan self-host in X86ISelLowering.cpp. I should be able to get a reduction soon, but I'll probably end up reverting this soon.</div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Dec 11, 2016 at 6:52 PM, Sebastian Pop via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: spop<br>
Date: Sun Dec 11 20:52:51 2016<br>
New Revision: 289412<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=289412&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=289412&view=rev</a><br>
Log:<br>
[SCEVExpand] do not hoist divisions by zero (PR30935)<br>
<br>
SCEVExpand computes the insertion point for the components of a SCEV to be code<br>
generated. When it comes to generating code for a division, SCEVexpand would<br>
not be able to check (at compilation time) all the conditions necessary to avoid<br>
a division by zero. The patch disables hoisting of expressions containing<br>
divisions by anything other than non-zero constants in order to avoid hoisting<br>
these expressions past conditions that should hold before doing the division.<br>
<br>
The patch passes check-all on x86_64-linux.<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D27216" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D27216</a><br>
<br>
Added:<br>
llvm/trunk/test/Transforms/<wbr>LoopIdiom/pr30935.ll<br>
Modified:<br>
llvm/trunk/lib/Analysis/<wbr>ScalarEvolution.cpp<br>
llvm/trunk/lib/Analysis/<wbr>ScalarEvolutionExpander.cpp<br>
llvm/trunk/unittests/Analysis/<wbr>ScalarEvolutionTest.cpp<br>
<br>
Modified: llvm/trunk/lib/Analysis/<wbr>ScalarEvolution.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ScalarEvolution.cpp?rev=289412&r1=289411&r2=289412&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/<wbr>Analysis/ScalarEvolution.cpp?<wbr>rev=289412&r1=289411&r2=<wbr>289412&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/lib/Analysis/<wbr>ScalarEvolution.cpp (original)<br>
+++ llvm/trunk/lib/Analysis/<wbr>ScalarEvolution.cpp Sun Dec 11 20:52:51 2016<br>
@@ -2130,7 +2130,7 @@ const SCEV *ScalarEvolution::getAddExpr(<br>
}<br>
<br>
// Okay, check to see if the same value occurs in the operand list more than<br>
- // once. If so, merge them together into an multiply expression. Since we<br>
+ // once. If so, merge them together into a multiply expression. Since we<br>
// sorted the list, these values are required to be adjacent.<br>
Type *Ty = Ops[0]->getType();<br>
bool FoundMatch = false;<br>
<br>
Modified: llvm/trunk/lib/Analysis/<wbr>ScalarEvolutionExpander.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp?rev=289412&r1=289411&r2=289412&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/<wbr>Analysis/<wbr>ScalarEvolutionExpander.cpp?<wbr>rev=289412&r1=289411&r2=<wbr>289412&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/lib/Analysis/<wbr>ScalarEvolutionExpander.cpp (original)<br>
+++ llvm/trunk/lib/Analysis/<wbr>ScalarEvolutionExpander.cpp Sun Dec 11 20:52:51 2016<br>
@@ -166,6 +166,16 @@ Value *SCEVExpander::<wbr>InsertNoopCastOfTo(<br>
return ReuseOrCreateCast(I, Ty, Op, IP);<br>
}<br>
<br>
+// Return true when S may contain the value zero.<br>
+static bool mayBeValueZero(Value *V) {<br>
+ if (ConstantInt *C = dyn_cast<ConstantInt>(V))<br>
+ if (!C->isZero())<br>
+ return false;<br>
+<br>
+ // All other expressions may have a zero value.<br>
+ return true;<br>
+}<br>
+<br>
/// InsertBinop - Insert the specified binary operator, doing a small amount<br>
/// of work to avoid inserting an obviously redundant operation.<br>
Value *SCEVExpander::InsertBinop(<wbr>Instruction::BinaryOps Opcode,<br>
@@ -198,14 +208,17 @@ Value *SCEVExpander::InsertBinop(<wbr>Instruc<br>
DebugLoc Loc = Builder.GetInsertPoint()-><wbr>getDebugLoc();<br>
SCEVInsertPointGuard Guard(Builder, this);<br>
<br>
- // Move the insertion point out of as many loops as we can.<br>
- while (const Loop *L = SE.LI.getLoopFor(Builder.<wbr>GetInsertBlock())) {<br>
- if (!L->isLoopInvariant(LHS) || !L->isLoopInvariant(RHS)) break;<br>
- BasicBlock *Preheader = L->getLoopPreheader();<br>
- if (!Preheader) break;<br>
+ // Only move the insertion point up when it is not a division by zero.<br>
+ if (Opcode != Instruction::UDiv || !mayBeValueZero(RHS)) {<br>
+ // Move the insertion point out of as many loops as we can.<br>
+ while (const Loop *L = SE.LI.getLoopFor(Builder.<wbr>GetInsertBlock())) {<br>
+ if (!L->isLoopInvariant(LHS) || !L->isLoopInvariant(RHS)) break;<br>
+ BasicBlock *Preheader = L->getLoopPreheader();<br>
+ if (!Preheader) break;<br>
<br>
- // Ok, move up a level.<br>
- Builder.SetInsertPoint(<wbr>Preheader->getTerminator());<br>
+ // Ok, move up a level.<br>
+ Builder.SetInsertPoint(<wbr>Preheader->getTerminator());<br>
+ }<br>
}<br>
<br>
// If we haven't found this binop, insert it.<br>
@@ -1666,31 +1679,46 @@ Value *SCEVExpander::expand(const SCEV *<br>
// Compute an insertion point for this SCEV object. Hoist the instructions<br>
// as far out in the loop nest as possible.<br>
Instruction *InsertPt = &*Builder.GetInsertPoint();<br>
- for (Loop *L = SE.LI.getLoopFor(Builder.<wbr>GetInsertBlock());;<br>
- L = L->getParentLoop())<br>
- if (SE.isLoopInvariant(S, L)) {<br>
- if (!L) break;<br>
- if (BasicBlock *Preheader = L->getLoopPreheader())<br>
- InsertPt = Preheader->getTerminator();<br>
- else {<br>
- // LSR sets the insertion point for AddRec start/step values to the<br>
- // block start to simplify value reuse, even though it's an invalid<br>
- // position. SCEVExpander must correct for this in all cases.<br>
- InsertPt = &*L->getHeader()-><wbr>getFirstInsertionPt();<br>
+ bool SafeToHoist = !SCEVExprContains(S, [](const SCEV *S) {<br>
+ if (const auto *D = dyn_cast<SCEVUDivExpr>(S)) {<br>
+ if (const auto *SC = dyn_cast<SCEVConstant>(D-><wbr>getRHS()))<br>
+ // Division by non-zero constants can be hoisted.<br>
+ return SC->getValue()->isZero();<br>
+<br>
+ // All other divisions should not be moved as they may be divisions by<br>
+ // zero and should be kept within the conditions of the surrounding<br>
+ // loops that guard their execution (see PR30935.)<br>
+ return true;<br>
}<br>
- } else {<br>
- // If the SCEV is computable at this level, insert it into the header<br>
- // after the PHIs (and after any other instructions that we've inserted<br>
- // there) so that it is guaranteed to dominate any user inside the loop.<br>
- if (L && SE.hasComputableLoopEvolution(<wbr>S, L) && !PostIncLoops.count(L))<br>
- InsertPt = &*L->getHeader()-><wbr>getFirstInsertionPt();<br>
- while (InsertPt->getIterator() != Builder.GetInsertPoint() &&<br>
- (isInsertedInstruction(<wbr>InsertPt) ||<br>
- isa<DbgInfoIntrinsic>(<wbr>InsertPt))) {<br>
- InsertPt = &*std::next(InsertPt-><wbr>getIterator());<br>
+ return false;<br>
+ });<br>
+ if (SafeToHoist) {<br>
+ for (Loop *L = SE.LI.getLoopFor(Builder.<wbr>GetInsertBlock());;<br>
+ L = L->getParentLoop())<br>
+ if (SE.isLoopInvariant(S, L)) {<br>
+ if (!L) break;<br>
+ if (BasicBlock *Preheader = L->getLoopPreheader())<br>
+ InsertPt = Preheader->getTerminator();<br>
+ else {<br>
+ // LSR sets the insertion point for AddRec start/step values to the<br>
+ // block start to simplify value reuse, even though it's an invalid<br>
+ // position. SCEVExpander must correct for this in all cases.<br>
+ InsertPt = &*L->getHeader()-><wbr>getFirstInsertionPt();<br>
+ }<br>
+ } else {<br>
+ // If the SCEV is computable at this level, insert it into the header<br>
+ // after the PHIs (and after any other instructions that we've inserted<br>
+ // there) so that it is guaranteed to dominate any user inside the loop.<br>
+ if (L && SE.hasComputableLoopEvolution(<wbr>S, L) && !PostIncLoops.count(L))<br>
+ InsertPt = &*L->getHeader()-><wbr>getFirstInsertionPt();<br>
+ while (InsertPt->getIterator() != Builder.GetInsertPoint() &&<br>
+ (isInsertedInstruction(<wbr>InsertPt) ||<br>
+ isa<DbgInfoIntrinsic>(<wbr>InsertPt))) {<br>
+ InsertPt = &*std::next(InsertPt-><wbr>getIterator());<br>
+ }<br>
+ break;<br>
}<br>
- break;<br>
- }<br>
+ }<br>
<br>
// Check to see if we already expanded this here.<br>
auto I = InsertedExpressions.find(std::<wbr>make_pair(S, InsertPt));<br>
<br>
Added: llvm/trunk/test/Transforms/<wbr>LoopIdiom/pr30935.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/LoopIdiom/pr30935.ll?rev=289412&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/test/<wbr>Transforms/LoopIdiom/pr30935.<wbr>ll?rev=289412&view=auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/test/Transforms/<wbr>LoopIdiom/pr30935.ll (added)<br>
+++ llvm/trunk/test/Transforms/<wbr>LoopIdiom/pr30935.ll Sun Dec 11 20:52:51 2016<br>
@@ -0,0 +1,94 @@<br>
+; RUN: opt -loop-idiom -S < %s | FileCheck %s<br>
+<br>
+; CHECK-LABEL: define i32 @main(<br>
+; CHECK: udiv<br>
+; CHECK-NOT: udiv<br>
+; CHECK: call void @llvm.memset.p0i8.i64<br>
+<br>
+@a = common local_unnamed_addr global [4 x i8] zeroinitializer, align 1<br>
+@b = common local_unnamed_addr global i32 0, align 4<br>
+@c = common local_unnamed_addr global i32 0, align 4<br>
+@d = common local_unnamed_addr global i32 0, align 4<br>
+@e = common local_unnamed_addr global i32 0, align 4<br>
+@f = common local_unnamed_addr global i32 0, align 4<br>
+@g = common local_unnamed_addr global i32 0, align 4<br>
+@h = common local_unnamed_addr global i64 0, align 8<br>
+<br>
+<br>
+define i32 @main() local_unnamed_addr #0 {<br>
+entry:<br>
+ %0 = load i32, i32* @e, align 4<br>
+ %tobool19 = icmp eq i32 %0, 0<br>
+ %1 = load i32, i32* @c, align 4<br>
+ %cmp10 = icmp eq i32 %1, 0<br>
+ %2 = load i32, i32* @g, align 4<br>
+ %3 = load i32, i32* @b, align 4<br>
+ %tobool = icmp eq i32 %0, 0<br>
+ br label %for.cond<br>
+<br>
+for.cond.loopexit: ; preds = %for.inc14<br>
+ br label %for.cond.backedge<br>
+<br>
+for.cond: ; preds = %for.cond.backedge, %entry<br>
+ %.pr = load i32, i32* @f, align 4<br>
+ %cmp20 = icmp eq i32 %.pr, 0<br>
+ br i1 %cmp20, label %for.cond2.preheader.<wbr>preheader, label %for.cond.backedge<br>
+<br>
+for.cond.backedge: ; preds = %for.cond, %for.cond.loopexit<br>
+ br label %for.cond<br>
+<br>
+for.cond2.preheader.<wbr>preheader: ; preds = %for.cond<br>
+ br label %for.cond2.preheader<br>
+<br>
+for.cond2.preheader: ; preds = %for.cond2.preheader.<wbr>preheader, %for.inc14<br>
+ br i1 %tobool19, label %for.cond9, label %<a href="http://for.body3.lr.ph" rel="noreferrer" target="_blank">for.body3.lr.ph</a><br>
+<br>
+<a href="http://for.body3.lr.ph" rel="noreferrer" target="_blank">for.body3.lr.ph</a>: ; preds = %for.cond2.preheader<br>
+ %div = udiv i32 %2, %3<br>
+ %conv = zext i32 %div to i64<br>
+ br label %for.body3<br>
+<br>
+for.cond4.for.cond2.loopexit_<wbr>crit_edge: ; preds = %for.body6<br>
+ store i32 0, i32* @d, align 4<br>
+ br label %for.cond2.loopexit<br>
+<br>
+for.cond2.loopexit: ; preds = %for.cond4.for.cond2.loopexit_<wbr>crit_edge, %for.body3<br>
+ br i1 %tobool, label %for.cond2.for.cond9_crit_<wbr>edge, label %for.body3<br>
+<br>
+for.body3: ; preds = %<a href="http://for.body3.lr.ph" rel="noreferrer" target="_blank">for.body3.lr.ph</a>, %for.cond2.loopexit<br>
+ %.pr17 = load i32, i32* @d, align 4<br>
+ %tobool518 = icmp eq i32 %.pr17, 0<br>
+ br i1 %tobool518, label %for.cond2.loopexit, label %for.body6.preheader<br>
+<br>
+for.body6.preheader: ; preds = %for.body3<br>
+ %4 = zext i32 %.pr17 to i64<br>
+ br label %for.body6<br>
+<br>
+for.body6: ; preds = %for.body6.preheader, %for.body6<br>
+ %indvars.iv = phi i64 [ %4, %for.body6.preheader ], [ %indvars.iv.next, %for.body6 ]<br>
+ %add = add nuw nsw i64 %conv, %indvars.iv<br>
+ %arrayidx = getelementptr inbounds [4 x i8], [4 x i8]* @a, i64 0, i64 %add<br>
+ store i8 1, i8* %arrayidx, align 1<br>
+ %5 = trunc i64 %indvars.iv to i32<br>
+ %inc = add i32 %5, 1<br>
+ %tobool5 = icmp eq i32 %inc, 0<br>
+ %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1<br>
+ br i1 %tobool5, label %for.cond4.for.cond2.loopexit_<wbr>crit_edge, label %for.body6<br>
+<br>
+for.cond2.for.cond9_crit_<wbr>edge: ; preds = %for.cond2.loopexit<br>
+ store i64 %conv, i64* @h, align 8<br>
+ br label %for.cond9<br>
+<br>
+for.cond9: ; preds = %for.cond2.for.cond9_crit_<wbr>edge, %for.cond2.preheader<br>
+ br i1 %cmp10, label %for.body12, label %for.inc14<br>
+<br>
+for.body12: ; preds = %for.cond9<br>
+ ret i32 0<br>
+<br>
+for.inc14: ; preds = %for.cond9<br>
+ %6 = load i32, i32* @f, align 4<br>
+ %inc15 = add i32 %6, 1<br>
+ store i32 %inc15, i32* @f, align 4<br>
+ %cmp = icmp eq i32 %inc15, 0<br>
+ br i1 %cmp, label %for.cond2.preheader, label %for.cond.loopexit<br>
+}<br>
<br>
Modified: llvm/trunk/unittests/Analysis/<wbr>ScalarEvolutionTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Analysis/ScalarEvolutionTest.cpp?rev=289412&r1=289411&r2=289412&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/unittests/<wbr>Analysis/ScalarEvolutionTest.<wbr>cpp?rev=289412&r1=289411&r2=<wbr>289412&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/unittests/Analysis/<wbr>ScalarEvolutionTest.cpp (original)<br>
+++ llvm/trunk/unittests/Analysis/<wbr>ScalarEvolutionTest.cpp Sun Dec 11 20:52:51 2016<br>
@@ -349,6 +349,13 @@ static Instruction *getInstructionByName<br>
llvm_unreachable("Expected to find instruction!");<br>
}<br>
<br>
+static Argument *getArgByName(Function &F, StringRef Name) {<br>
+ for (auto &A : F.args())<br>
+ if (A.getName() == Name)<br>
+ return &A;<br>
+ llvm_unreachable("Expected to find argument!");<br>
+}<br>
+<br>
TEST_F(ScalarEvolutionsTest, CommutativeExprOperandOrder) {<br>
LLVMContext C;<br>
SMDiagnostic Err;<br>
@@ -532,5 +539,74 @@ TEST_F(ScalarEvolutionsTest, SCEVCompare<br>
EXPECT_NE(nullptr, SE.getSCEV(Acc[0]));<br>
}<br>
<br>
+TEST_F(ScalarEvolutionsTest, BadHoistingSCEVExpander_<wbr>PR30942) {<br>
+ LLVMContext C;<br>
+ SMDiagnostic Err;<br>
+ std::unique_ptr<Module> M = parseAssemblyString(<br>
+ "target datalayout = \"e-m:e-p:32:32-f64:32:64-f80:<wbr>32-n8:16:32-S128\" "<br>
+ " "<br>
+ "define void @f_1(i32 %x, i32 %y, i32 %n, i1* %cond_buf) "<br>
+ " local_unnamed_addr { "<br>
+ "entry: "<br>
+ " %entrycond = icmp sgt i32 %n, 0 "<br>
+ " br i1 %entrycond, label %<a href="http://loop.ph" rel="noreferrer" target="_blank">loop.ph</a>, label %for.end "<br>
+ " "<br>
+ "<a href="http://loop.ph" rel="noreferrer" target="_blank">loop.ph</a>: "<br>
+ " br label %loop "<br>
+ " "<br>
+ "loop: "<br>
+ " %iv1 = phi i32 [ %iv1.inc, %right ], [ 0, %<a href="http://loop.ph" rel="noreferrer" target="_blank">loop.ph</a> ] "<br>
+ " %iv1.inc = add nuw nsw i32 %iv1, 1 "<br>
+ " %cond = load volatile i1, i1* %cond_buf "<br>
+ " br i1 %cond, label %left, label %right "<br>
+ " "<br>
+ "left: "<br>
+ " %div = udiv i32 %x, %y "<br>
+ " br label %right "<br>
+ " "<br>
+ "right: "<br>
+ " %exitcond = icmp eq i32 %iv1.inc, %n "<br>
+ " br i1 %exitcond, label %for.end.loopexit, label %loop "<br>
+ " "<br>
+ "for.end.loopexit: "<br>
+ " br label %for.end "<br>
+ " "<br>
+ "for.end: "<br>
+ " ret void "<br>
+ "} ",<br>
+ Err, C);<br>
+<br>
+ assert(M && "Could not parse module?");<br>
+ assert(!verifyModule(*M) && "Must have been well formed!");<br>
+<br>
+ runWithFunctionAndSE(*M, "f_1", [&](Function &F, ScalarEvolution &SE) {<br>
+ SCEVExpander Expander(SE, M->getDataLayout(), "unittests");<br>
+ auto *DivInst = getInstructionByName(F, "div");<br>
+<br>
+ {<br>
+ auto *DivSCEV = SE.getSCEV(DivInst);<br>
+ auto *DivExpansion = Expander.expandCodeFor(<br>
+ DivSCEV, DivSCEV->getType(), DivInst->getParent()-><wbr>getTerminator());<br>
+ auto *DivExpansionInst = dyn_cast<Instruction>(<wbr>DivExpansion);<br>
+ ASSERT_NE(DivExpansionInst, nullptr);<br>
+ EXPECT_EQ(DivInst->getParent()<wbr>, DivExpansionInst->getParent())<wbr>;<br>
+ }<br>
+<br>
+ {<br>
+ auto *ArgY = getArgByName(F, "y");<br>
+ auto *DivFromScratchSCEV =<br>
+ SE.getUDivExpr(SE.getOne(ArgY-<wbr>>getType()), SE.getSCEV(ArgY));<br>
+<br>
+ auto *DivFromScratchExpansion = Expander.expandCodeFor(<br>
+ DivFromScratchSCEV, DivFromScratchSCEV->getType(),<br>
+ DivInst->getParent()-><wbr>getTerminator());<br>
+ auto *DivFromScratchExpansionInst =<br>
+ dyn_cast<Instruction>(<wbr>DivFromScratchExpansion);<br>
+ ASSERT_NE(<wbr>DivFromScratchExpansionInst, nullptr);<br>
+ EXPECT_EQ(DivInst->getParent()<wbr>, DivFromScratchExpansionInst-><wbr>getParent());<br>
+ }<br>
+ });<br>
+}<br>
+<br>
} // end anonymous namespace<br>
} // end namespace llvm<br>
<br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div>