[llvm] r289412 - [SCEVExpand] do not hoist divisions by zero (PR30935)

Reid Kleckner via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 12 15:33:55 PST 2016


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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161212/04aecd00/attachment.html>


More information about the llvm-commits mailing list