[PATCH] D92247: [IR][LoopRotate] avoid leaving phi with no operands (PR48296)

[Sanjay Patel via Phabricator via llvm-commits]

spatel created this revision.
spatel added reviewers: foad, lebedev.ri, lattner.
Herald added subscribers: dexonsmith, hiraditya, mcrosier.
Herald added a project: LLVM.
spatel requested review of this revision.

https://llvm.org/PR48296 shows an example where we delete all of the operands of a phi without actually deleting the phi, and that is invalid IR. The reduced test included here currently crashes for that reason. This was updated relatively recently in D80141 <https://reviews.llvm.org/D80141>.

I'm not familiar with all of the callers and use cases, but it seems there are passes that expect to handle deleting such a phi at a later time. We get regression test failures if we just stub out the DeletePHIIfEmpty argument in removeIncomingValue() for example.

So I changed the logic in BasicBlock::removePredecessor() to allow deletion of the phi if it has only one predecessor to start with because removing the lone predecessor is guaranteed to produce invalid IR in that case. This does not appear to cause problems in other passes.


https://reviews.llvm.org/D92247

Files:
  llvm/lib/IR/BasicBlock.cpp
  llvm/test/Transforms/LoopRotate/phi-empty.ll


Index: llvm/test/Transforms/LoopRotate/phi-empty.ll
===================================================================
--- /dev/null
+++ llvm/test/Transforms/LoopRotate/phi-empty.ll
@@ -0,0 +1,35 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -lcssa -loop-rotate < %s | FileCheck %s
+
+define void @PR48296(i1 %cond) {
+; CHECK-LABEL: @PR48296(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[LOOP:%.*]]
+; CHECK:       loop:
+; CHECK-NEXT:    br i1 [[COND:%.*]], label [[INC:%.*]], label [[LOOP_BACKEDGE:%.*]]
+; CHECK:       loop.backedge:
+; CHECK-NEXT:    br label [[LOOP]]
+; CHECK:       dead:
+; CHECK-NEXT:    unreachable
+; CHECK:       inc:
+; CHECK-NEXT:    br label [[LOOP_BACKEDGE]]
+; CHECK:       return:
+; CHECK-NEXT:    ret void
+;
+entry:
+  br label %loop
+
+loop:
+  br i1 %cond, label %inc, label %loop
+
+dead:                                        ; No predecessors!
+  br i1 %cond, label %inc, label %return
+
+inc:
+  br label %loop
+
+return:
+  %r = phi i32 [ undef, %dead ]
+  ret void
+}
+
Index: llvm/lib/IR/BasicBlock.cpp
===================================================================
--- llvm/lib/IR/BasicBlock.cpp
+++ llvm/lib/IR/BasicBlock.cpp
@@ -332,9 +332,9 @@
 /// Update PHI nodes in this BasicBlock before removal of predecessor \p Pred.
 /// Note that this function does not actually remove the predecessor.
 ///
-/// If \p KeepOneInputPHIs is true then don't remove PHIs that are left with
-/// zero or one incoming values, and don't simplify PHIs with all incoming
-/// values the same.
+/// If \p KeepOneInputPHIs is true, then don't remove PHIs that are left with
+/// one incoming value, and don't simplify PHIs with all incoming values that
+/// are the same constant.
 void BasicBlock::removePredecessor(BasicBlock *Pred,
                                    bool KeepOneInputPHIs) {
   // Use hasNUsesOrMore to bound the cost of this assertion for complex CFGs.
@@ -349,7 +349,8 @@
   // Update all PHI nodes.
   for (iterator II = begin(); isa<PHINode>(II);) {
     PHINode *PN = cast<PHINode>(II++);
-    PN->removeIncomingValue(Pred, !KeepOneInputPHIs);
+    bool DeletePhiIfEmpty = NumPreds == 1 || !KeepOneInputPHIs;
+    PN->removeIncomingValue(Pred, DeletePhiIfEmpty);
     if (!KeepOneInputPHIs) {
       // If we have a single predecessor, removeIncomingValue erased the PHI
       // node itself.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D92247.308108.patch
Type: text/x-patch
Size: 2413 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201127/278fb16c/attachment.bin>