[PATCH] D23464: [PR28367][Reassociation] Avoid iterator invalidation when negating value.

[Chad Rosier via llvm-commits]

mcrosier created this revision.
mcrosier added reviewers: aditya_nandakumar, gberry, hans.
mcrosier added a subscriber: llvm-commits.

When trying to negate a value, V, we try to avoid materializing the negated value by scanning the use list of V to see if we already have one.  If we do find a negated version and that version is in the block we're currently optimizing, moving that instruction may invalidate our iterator.

Example:
define void @fn1(i32 %a, i1 %c, i32* %ptr)  {
entry:
  br label %for.cond

for.cond:
  %d.0 = phi i32 [ 1, %entry ], [ 2, %for.body ]
  br i1 %c, label %for.end, label %for.body

for.body:
  %sub1 = sub i32 %a, %d.0
  %dead1 = add i32 %sub1, 1
  %dead2 = mul i32 %dead1, 3
  %dead3 = mul i32 %dead2, %sub1
  %sub2 = sub nsw i32 0, %d.0
  store i32 %sub2, i32* %ptr, align 4
  br label %for.cond

for.end:
  ret void
}

The invalidation occurs as follows:
1. Assume we're optimizing %dead3, which is trivially dead.
2. The BB iterator is incremented (now pointing to %sub2) and EraseInst is called to delete %dead3.
3. When the %dead3 instruction is removed its operand %sub1 is added to the RedoInst list.
4. When %sub1 is reoptimized the ShouldBreakUpSubtract() function returns true and the BreakUpSubtract() function is called.  This didn't happen the first time %sub1 was optimized because it had more than one use (i.e., %dead1 and %dead3).
5. The NegateValue() function then hoists the %sub2 (as it is the negated form of %d.0) instruction into the for.cond block invalidating the iterator and causing the assertion.  Without the assert we would dereference an invalid iterator and segfault.

Please take a look,
 Chad

https://reviews.llvm.org/D23464

Files:
  lib/Transforms/Scalar/Reassociate.cpp
  test/Transforms/Reassociate/invalid-iterator.ll

Index: test/Transforms/Reassociate/invalid-iterator.ll
===================================================================
--- /dev/null
+++ test/Transforms/Reassociate/invalid-iterator.ll
@@ -0,0 +1,30 @@
+; RUN: opt < %s -reassociate -die -S | FileCheck %s
+
+; PR28367
+; Check to make sure %sub2 isn't moved from for.body.  Doing so would invalidate
+; the iterator.
+
+; CHECK-LABEL: @fn1
+; CHECK: for.body:
+; CHECK-NEXT:   %sub2 = sub nsw i32 0, %d.0
+; CHECK-NEXT:   store i32 %sub2, i32* %ptr, align 4
+define void @fn1(i32 %a, i1 %c, i32* %ptr)  {
+entry:
+  br label %for.cond
+
+for.cond:
+  %d.0 = phi i32 [ 1, %entry ], [ 2, %for.body ]
+  br i1 %c, label %for.end, label %for.body
+
+for.body:
+  %sub1 = sub i32 %a, %d.0
+  %dead1 = add i32 %sub1, 1
+  %dead2 = mul i32 %dead1, 3
+  %dead3 = mul i32 %dead2, %sub1
+  %sub2 = sub nsw i32 0, %d.0
+  store i32 %sub2, i32* %ptr, align 4
+  br label %for.cond
+
+for.end:
+  ret void
+}
Index: lib/Transforms/Scalar/Reassociate.cpp
===================================================================
--- lib/Transforms/Scalar/Reassociate.cpp
+++ lib/Transforms/Scalar/Reassociate.cpp
@@ -842,6 +842,11 @@
     if (TheNeg->getParent()->getParent() != BI->getParent()->getParent())
       continue;
 
+    // Don't move the negate if it's in the block we're currently optimizing as
+    // this may invalidate our iterator.
+    if (TheNeg->getParent() == BI->getParent())
+      continue;
+
     BasicBlock::iterator InsertPt;
     if (Instruction *InstInput = dyn_cast<Instruction>(V)) {
       if (InvokeInst *II = dyn_cast<InvokeInst>(InstInput)) {
@@ -1863,6 +1868,8 @@
 /// Zap the given instruction, adding interesting operands to the work list.
 void ReassociatePass::EraseInst(Instruction *I) {
   assert(isInstructionTriviallyDead(I) && "Trivially dead instructions only!");
+  DEBUG(dbgs() << "Erasing dead inst: "; I->dump());
+
   SmallVector<Value*, 8> Ops(I->op_begin(), I->op_end());
   // Erase the dead instruction.
   ValueRankMap.erase(I);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D23464.67903.patch
Type: text/x-patch
Size: 2021 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160812/03e8eccf/attachment.bin>