[PATCH] D65336: [InstSimplify] remove quadratic time looping (PR42771)

[Sanjay Patel via Phabricator via llvm-commits]

spatel created this revision.
spatel added reviewers: foad, Gerolf, lebedev.ri.
Herald added subscribers: hiraditya, mcrosier.
Herald added a project: LLVM.

The test case from:
https://bugs.llvm.org/show_bug.cgi?id=42771
...shows a ~30x slowdown caused by the awkward loop iteration (rL207302 <https://reviews.llvm.org/rL207302>) that is seemingly done just to avoid invalidating the instruction iterator. We can instead delay instruction deletion until we reach the end of the block (or we could delay until we reach the end of all blocks).

There's a test diff here for a degenerate case with llvm.assume that I didn't step through, but I think that's caused because isInstructionTriviallyDead() may not be true for a function call even if that call has no uses.

This change probably doesn't result in much overall compile-time improvement because we call -instsimplify as a standalone pass only once in the standard -O2 opt pipeline from what I see.


https://reviews.llvm.org/D65336

Files:
  llvm/lib/Transforms/Scalar/InstSimplifyPass.cpp
  llvm/test/Transforms/InstSimplify/assume.ll


Index: llvm/test/Transforms/InstSimplify/assume.ll
===================================================================
--- llvm/test/Transforms/InstSimplify/assume.ll
+++ llvm/test/Transforms/InstSimplify/assume.ll
@@ -33,15 +33,15 @@
 }
 
 ; Similar to above: there's no way to know which assumption is truthful,
-; so just don't crash. The second icmp+assume gets processed later, so that
-; determines the return value.
+; so just don't crash.
 
 define i8 @conflicting_assumptions(i8 %x) !dbg !10 {
 ; CHECK-LABEL: @conflicting_assumptions(
+; CHECK-NEXT:    [[ADD:%.*]] = add i8 [[X:%.*]], 1, !dbg !10
 ; CHECK-NEXT:    call void @llvm.assume(i1 false)
-; CHECK-NEXT:    [[COND2:%.*]] = icmp eq i8 %x, 4
+; CHECK-NEXT:    [[COND2:%.*]] = icmp eq i8 [[X]], 4
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[COND2]])
-; CHECK-NEXT:    ret i8 5
+; CHECK-NEXT:    ret i8 [[ADD]]
 ;
   %add = add i8 %x, 1, !dbg !11
   %cond1 = icmp eq i8 %x, 3
Index: llvm/lib/Transforms/Scalar/InstSimplifyPass.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/InstSimplifyPass.cpp
+++ llvm/lib/Transforms/Scalar/InstSimplifyPass.cpp
@@ -33,37 +33,31 @@
   bool Changed = false;
 
   do {
-    for (BasicBlock *BB : depth_first(&F.getEntryBlock())) {
-      // Here be subtlety: the iterator must be incremented before the loop
-      // body (not sure why), so a range-for loop won't work here.
-      for (BasicBlock::iterator BI = BB->begin(), BE = BB->end(); BI != BE;) {
-        Instruction *I = &*BI++;
-        // The first time through the loop ToSimplify is empty and we try to
-        // simplify all instructions.  On later iterations ToSimplify is not
+    for (BasicBlock &BB : F) {
+      SmallVector<Instruction *, 8> DeadInstsInBB;
+      for (Instruction &I : BB) {
+        // The first time through the loop, ToSimplify is empty and we try to
+        // simplify all instructions. On later iterations, ToSimplify is not
         // empty and we only bother simplifying instructions that are in it.
-        if (!ToSimplify->empty() && !ToSimplify->count(I))
+        if (!ToSimplify->empty() && !ToSimplify->count(&I))
           continue;
 
         // Don't waste time simplifying unused instructions.
-        if (!I->use_empty()) {
-          if (Value *V = SimplifyInstruction(I, SQ, ORE)) {
-            // Mark all uses for resimplification next time round the loop.
-            for (User *U : I->users())
-              Next->insert(cast<Instruction>(U));
-            I->replaceAllUsesWith(V);
-            ++NumSimplified;
-            Changed = true;
-          }
-        }
-        if (RecursivelyDeleteTriviallyDeadInstructions(I, SQ.TLI)) {
-          // RecursivelyDeleteTriviallyDeadInstruction can remove more than one
-          // instruction, so simply incrementing the iterator does not work.
-          // When instructions get deleted re-iterate instead.
-          BI = BB->begin();
-          BE = BB->end();
+        if (isInstructionTriviallyDead(&I)) {
+          DeadInstsInBB.push_back(&I);
+        } else if (Value *V = SimplifyInstruction(&I, SQ, ORE)) {
+          // Mark all uses for resimplification next time round the loop.
+          for (User *U : I.users())
+            Next->insert(cast<Instruction>(U));
+          I.replaceAllUsesWith(V);
+          ++NumSimplified;
           Changed = true;
+          // A call can get simplified, but it may not be trivially dead.
+          if (isInstructionTriviallyDead(&I))
+            DeadInstsInBB.push_back(&I);
         }
       }
+      RecursivelyDeleteTriviallyDeadInstructions(DeadInstsInBB, SQ.TLI);
     }
 
     // Place the list of instructions to simplify on the next loop iteration


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D65336.211952.patch
Type: text/x-patch
Size: 3745 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190726/97021aa6/attachment.bin>