[InstCombine] Remove dead phi web

[Chengjunp]

In current visitPHINode function during InstCombine, it can remove dead phi cycles (all phis have one use, which is another phi). However, it cannot deal with the case when the phis form a web (all phis have one or more uses, and all the uses are phi). This change extends the algorithm so that it can also deal with the dead phi web.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: None (Chengjunp)

Changes

In current visitPHINode function during InstCombine, it can remove dead phi cycles (all phis have one use, which is another phi). However, it cannot deal with the case when the phis form a web (all phis have one or more uses, and all the uses are phi). This change extends the algorithm so that it can also deal with the dead phi web.

---

Full diff: https://github.com/llvm/llvm-project/pull/108876.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp (+28-29)
• (modified) llvm/test/Transforms/InstCombine/phi.ll (+51)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp b/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
index bcff9a72b65724..697e7b35033cd9 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
@@ -976,24 +976,26 @@ Instruction *InstCombinerImpl::foldPHIArgOpIntoPHI(PHINode &PN) {
   return NewCI;
 }
 
-/// Return true if this PHI node is only used by a PHI node cycle that is dead.
-static bool isDeadPHICycle(PHINode *PN,
-                           SmallPtrSetImpl<PHINode *> &PotentiallyDeadPHIs) {
-  if (PN->use_empty()) return true;
-  if (!PN->hasOneUse()) return false;
-
-  // Remember this node, and if we find the cycle, return.
-  if (!PotentiallyDeadPHIs.insert(PN).second)
-    return true;
-
-  // Don't scan crazily complex things.
-  if (PotentiallyDeadPHIs.size() == 16)
-    return false;
-
-  if (PHINode *PU = dyn_cast<PHINode>(PN->user_back()))
-    return isDeadPHICycle(PU, PotentiallyDeadPHIs);
-
-  return false;
+/// Return true if this PHI node is only used by a PHI node web that is dead.
+static bool isDeadPHIWeb(PHINode *PN) {
+  SmallVector<PHINode *, 16> Stack;
+  SmallPtrSet<PHINode *, 16> Visited;
+  Stack.push_back(PN);
+  while (!Stack.empty()) {
+    PHINode *Phi = Stack.pop_back_val();
+    if (!Visited.insert(Phi).second)
+      continue;
+    // Early stop if the set of PHIs is large
+    if (Visited.size() == 16)
+      return false;
+    for (User *Use : Phi->users()) {
+      PHINode *PhiUse = dyn_cast<PHINode>(Use);
+      if (!PhiUse)
+        return false;
+      Stack.push_back(PhiUse);
+    }
+  }
+  return true;
 }
 
 /// Return true if this phi node is always equal to NonPhiInVal.
@@ -1474,27 +1476,24 @@ Instruction *InstCombinerImpl::visitPHINode(PHINode &PN) {
     }
   }
 
-  // If this is a trivial cycle in the PHI node graph, remove it.  Basically, if
-  // this PHI only has a single use (a PHI), and if that PHI only has one use (a
-  // PHI)... break the cycle.
+  // If the phi is within a phi web, which is formed by the def-use chain
+  // of phis and all the phis in the web are only used in the other phis. In
+  // this case, these phis are dead. We can break the web by removing PN.
+  if (isDeadPHIWeb(&PN))
+    return replaceInstUsesWith(PN, PoisonValue::get(PN.getType()));
+
+  // Optimization when the phi only has one use
   if (PN.hasOneUse()) {
     if (foldIntegerTypedPHI(PN))
       return nullptr;
 
-    Instruction *PHIUser = cast<Instruction>(PN.user_back());
-    if (PHINode *PU = dyn_cast<PHINode>(PHIUser)) {
-      SmallPtrSet<PHINode*, 16> PotentiallyDeadPHIs;
-      PotentiallyDeadPHIs.insert(&PN);
-      if (isDeadPHICycle(PU, PotentiallyDeadPHIs))
-        return replaceInstUsesWith(PN, PoisonValue::get(PN.getType()));
-    }
-
     // If this phi has a single use, and if that use just computes a value for
     // the next iteration of a loop, delete the phi.  This occurs with unused
     // induction variables, e.g. "for (int j = 0; ; ++j);".  Detecting this
     // common case here is good because the only other things that catch this
     // are induction variable analysis (sometimes) and ADCE, which is only run
     // late.
+    Instruction *PHIUser = cast<Instruction>(PN.user_back());
     if (PHIUser->hasOneUse() &&
         (isa<BinaryOperator>(PHIUser) || isa<UnaryOperator>(PHIUser) ||
          isa<GetElementPtrInst>(PHIUser)) &&
diff --git a/llvm/test/Transforms/InstCombine/phi.ll b/llvm/test/Transforms/InstCombine/phi.ll
index 3b1fa3a97d9cd7..b33ad9a7d339f2 100644
--- a/llvm/test/Transforms/InstCombine/phi.ll
+++ b/llvm/test/Transforms/InstCombine/phi.ll
@@ -2742,3 +2742,54 @@ loop.latch:
   call void @use(i32 %and)
   br label %loop
 }
+
+define void @test_dead_phi_web(i64 %index, i1 %cond) {
+; CHECK-LABEL: @test_dead_phi_web(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[BB0:%.*]]
+; CHECK:       BB0:
+; CHECK-NEXT:    switch i64 [[INDEX:%.*]], label [[BB4:%.*]] [
+; CHECK-NEXT:      i64 0, label [[BB1:%.*]]
+; CHECK-NEXT:      i64 1, label [[BB2:%.*]]
+; CHECK-NEXT:      i64 2, label [[BB3:%.*]]
+; CHECK-NEXT:    ]
+; CHECK:       BB1:
+; CHECK-NEXT:    br i1 [[COND:%.*]], label [[BB2]], label [[BB4]]
+; CHECK:       BB2:
+; CHECK-NEXT:    br i1 [[COND]], label [[BB3]], label [[BB4]]
+; CHECK:       BB3:
+; CHECK-NEXT:    br label [[BB4]]
+; CHECK:       BB4:
+; CHECK-NEXT:    br i1 [[COND]], label [[BB0]], label [[BB5:%.*]]
+; CHECK:       BB5:
+; CHECK-NEXT:    ret void
+;
+entry:
+  br label %BB0
+
+BB0:                                              ; preds = %BB4, %entry
+  %a = phi float [ 0.0, %entry ], [ %x, %BB4 ]
+  switch i64 %index, label %BB4 [
+  i64 0, label %BB1
+  i64 1, label %BB2
+  i64 2, label %BB3
+  ]
+
+BB1:                                              ; preds = %BB0
+  br i1 %cond, label %BB2, label %BB4
+
+BB2:                                              ; preds = %BB1, %BB0
+  %b = phi float [ 2.0, %BB0 ], [ %a, %BB1 ]
+  br i1 %cond, label %BB3, label %BB4
+
+BB3:                                              ; preds = %BB2, %BB0
+  %c = phi float [ 3.0, %BB0 ], [ %b, %BB2 ]
+  br label %BB4
+
+BB4:                                             ; preds = %BB3, %BB2, %BB1, %BB0
+  %x = phi float [ %a, %BB0 ], [ %a, %BB1 ], [ %b, %BB2 ], [ %c, %BB3 ]
+  br i1 %cond, label %BB0, label %BB5
+
+BB5:                                             ; preds = %BB4
+  ret void
+}

[nikic]

LGTM. This seems like a reasonable extension, and the number of visited phi nodes is still limited to a small number. Compile-time impact looks ok: https://llvm-compile-time-tracker.com/compare.php?from=223e2efa5e886502a9467b7ef700ebce9b7886e8&to=55a38338a2ebeef31ad8da34d965763f522e5799&stat=instructions:u

[dtcxzyw]

Why not just delete the whole dead phi web here? Thus we can avoid O(n^2) traversals.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[dtcxzyw]

LGTM. Thank you!