[llvm] r289412 - [SCEVExpand] do not hoist divisions by zero (PR30935)
Mike Aizatsky via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 12 16:14:45 PST 2016
BTW
This (or http://llvm.org/viewvc/llvm-project?revision=289413&view=revision -
can't tell for sure don't have windows) has also broken sanitizer-windows
bot:
http://lab.llvm.org:8011/builders/sanitizer-windows/builds/2786
FAILED:
projects/compiler-rt/lib/sanitizer_common/tests/sanitizer_stacktrace_test.cc.i386.o
cmd.exe /C "cd /D
C:\b\slave\sanitizer-windows\build\projects\compiler-rt\lib\sanitizer_common\tests
&& C:\b\slave\sanitizer-windows\build\.\bin\clang.exe -DWIN32 -D_WINDOWS
-Wno-unknown-warning-option -fms-compatibility-version=19.00.24215.1
-D_HAS_EXCEPTIONS=0 -Wno-undefined-inline -DGTEST_NO_LLVM_RAW_OSTREAM=1
-DGTEST_HAS_RTTI=0
-IC:/b/slave/sanitizer-windows/llvm/utils/unittest/googletest/include
-IC:/b/slave/sanitizer-windows/llvm/utils/unittest/googletest
-DGTEST_HAS_SEH=0 -Wno-deprecated-declarations
-IC:/b/slave/sanitizer-windows/llvm/projects/compiler-rt/include
-IC:/b/slave/sanitizer-windows/llvm/projects/compiler-rt/lib
-IC:/b/slave/sanitizer-windows/llvm/projects/compiler-rt/lib/sanitizer_common
-fno-rtti -O2 -Werror=sign-compare -Wno-non-virtual-dtor -fno-exceptions
-DGTEST_HAS_SEH=0 -gline-tables-only -gcodeview -c -o
sanitizer_stacktrace_test.cc.i386.o
C:/b/slave/sanitizer-windows/llvm/projects/compiler-rt/lib/sanitizer_common/tests/sanitizer_stacktrace_test.cc"
Assertion failed: (PtrWord & ~PointerBitMask) == 0 && "Pointer is not
sufficiently aligned", file
C:\b\slave\sanitizer-windows\llvm\include\llvm/ADT/PointerIntPair.h, line
160
Wrote crash dump file
"C:\Users\buildbot\AppData\Local\Temp\clang.exe-692761.dmp"
#0 0x01f8dd87 (C:\b\slave\sanitizer-windows\build\bin\clang.exe+0xd4dd87)
#1 0x70a64672 (C:\windows\SYSTEM32\ucrtbase.DLL+0x84672)
#2 0x70a65cab (C:\windows\SYSTEM32\ucrtbase.DLL+0x85cab)
#3 0x70a650e8 (C:\windows\SYSTEM32\ucrtbase.DLL+0x850e8)
#4 0x70a65da6 (C:\windows\SYSTEM32\ucrtbase.DLL+0x85da6)
#5 0x02ff860d (C:\b\slave\sanitizer-windows\build\bin\clang.exe+0x1db860d)
clang.exe: error: clang frontend command failed due to signal (use -v to
see invocation)
clang version 4.0.0 (trunk 289413)
Target: i686-pc-windows-msvc
Thread model: posix
InstalledDir: C:\b\slave\sanitizer-windows\build\bin
clang.exe: note: diagnostic msg: PLEASE submit a bug report to
http://llvm.org/bugs/ and include the crash backtrace, preprocessed source,
and associated run script.
clang.exe: note: diagnostic msg:
********************
On Mon, Dec 12, 2016 at 3:34 PM Reid Kleckner via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Reverted in r289453. This is still reducing, but here's a link to the
> partially reduced source if you want to take a look at it:
> https://drive.google.com/open?id=0B5-KodWdXF4YbHQ5RlJiaEQ2REE
>
> Compile with 'clang -c -fsanitize=address -O2 -std=c++11 -fno-exceptions
> test.cpp'
>
> On Mon, Dec 12, 2016 at 10:53 AM, Sanjoy Das <
> sanjoy at playingwithpointers.com> wrote:
>
> You may also need to revert rL289435 to avoid merge conflicts.
>
> Sent from a mobile device, please excuse typos.
>
> On Dec 12, 2016 10:34, "Reid Kleckner via llvm-commits"
> <llvm-commits at lists.llvm.org> wrote:
>
> 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.
>
> On Sun, Dec 11, 2016 at 6:52 PM, Sebastian Pop via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
> >
> > Author: spop
> > Date: Sun Dec 11 20:52:51 2016
> > New Revision: 289412
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=289412&view=rev
> > Log:
> > [SCEVExpand] do not hoist divisions by zero (PR30935)
> >
> > SCEVExpand computes the insertion point for the components of a SCEV to
> be code
> > generated. When it comes to generating code for a division, SCEVexpand
> would
> > not be able to check (at compilation time) all the conditions necessary
> to avoid
> > a division by zero. The patch disables hoisting of expressions
> containing
> > divisions by anything other than non-zero constants in order to avoid
> hoisting
> > these expressions past conditions that should hold before doing the
> division.
> >
> > The patch passes check-all on x86_64-linux.
> >
> > Differential Revision: https://reviews.llvm.org/D27216
> >
> > Added:
> > 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=289412&r1=289411&r2=289412&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/Analysis/ScalarEvolution.cpp (original)
> > +++ llvm/trunk/lib/Analysis/ScalarEvolution.cpp Sun Dec 11 20:52:51 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 an multiply expression.
> Since we
> > + // once. If so, merge them together into a 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=289412&r1=289411&r2=289412&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp (original)
> > +++ llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp Sun Dec 11
> 20:52:51 2016
> > @@ -166,6 +166,16 @@ 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,
> > @@ -198,14 +208,17 @@ Value *SCEVExpander::InsertBinop(Instruc
> > DebugLoc Loc = Builder.GetInsertPoint()->getDebugLoc();
> > SCEVInsertPointGuard Guard(Builder, this);
> >
> > - // 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;
> > + // 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;
> >
> > - // 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.
> > @@ -1666,31 +1679,46 @@ 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();
> > - 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();
> > + 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;
> > }
> > - } 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());
> > + 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;
> > }
> > - break;
> > - }
> > + }
> >
> > // Check to see if we already expanded this here.
> > auto I = InsertedExpressions.find(std::make_pair(S, InsertPt));
> >
> > Added: llvm/trunk/test/Transforms/LoopIdiom/pr30935.ll
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/LoopIdiom/pr30935.ll?rev=289412&view=auto
> >
> ==============================================================================
> > --- llvm/trunk/test/Transforms/LoopIdiom/pr30935.ll (added)
> > +++ llvm/trunk/test/Transforms/LoopIdiom/pr30935.ll Sun Dec 11 20:52:51
> 2016
> > @@ -0,0 +1,94 @@
> > +; 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=289412&r1=289411&r2=289412&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/unittests/Analysis/ScalarEvolutionTest.cpp (original)
> > +++ llvm/trunk/unittests/Analysis/ScalarEvolutionTest.cpp Sun Dec 11
> 20:52:51 2016
> > @@ -349,6 +349,13 @@ 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;
> > @@ -532,5 +539,74 @@ 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!");
> > +
> > + runWithFunctionAndSE(*M, "f_1", [&](Function &F, 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());
> > + }
> > + });
> > +}
> > +
> > } // end anonymous namespace
> > } // end namespace llvm
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
--
Mike
Sent from phone
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161213/4e11f9ad/attachment.html>
More information about the llvm-commits
mailing list