[UnifyLoopExits] Never generate phis of only undef values

[jdoerfert]

No description provided.

[llvmbot]

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-clang-codegen

Author: Johannes Doerfert (jdoerfert)

Changes

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Utils/BasicBlockUtils.cpp (+9-2)
• (added) llvm/test/Transforms/UnifyLoopExits/undef-phis.ll (+58)

diff --git a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
index 462283c0bfe00..4c8cfa5c6c697 100644
--- a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
+++ b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
@@ -1909,22 +1909,29 @@ static void reconnectPhis(BasicBlock *Out, BasicBlock *GuardBlock,
     auto NewPhi =
         PHINode::Create(Phi->getType(), Incoming.size(),
                         Phi->getName() + ".moved", FirstGuardBlock->begin());
+    bool AllUndef = true;
     for (auto *In : Incoming) {
       Value *V = UndefValue::get(Phi->getType());
       if (In == Out) {
         V = NewPhi;
       } else if (Phi->getBasicBlockIndex(In) != -1) {
         V = Phi->removeIncomingValue(In, false);
+        AllUndef &= isa<UndefValue>(V);
       }
       NewPhi->addIncoming(V, In);
     }
     assert(NewPhi->getNumIncomingValues() == Incoming.size());
+    Value *NewV = NewPhi;
+    if (AllUndef) {
+      NewPhi->eraseFromParent();
+      NewV = UndefValue::get(Phi->getType());
+    }
     if (Phi->getNumOperands() == 0) {
-      Phi->replaceAllUsesWith(NewPhi);
+      Phi->replaceAllUsesWith(NewV);
       I = Phi->eraseFromParent();
       continue;
     }
-    Phi->addIncoming(NewPhi, GuardBlock);
+    Phi->addIncoming(NewV, GuardBlock);
     ++I;
   }
 }
diff --git a/llvm/test/Transforms/UnifyLoopExits/undef-phis.ll b/llvm/test/Transforms/UnifyLoopExits/undef-phis.ll
new file mode 100644
index 0000000000000..722ccc519a764
--- /dev/null
+++ b/llvm/test/Transforms/UnifyLoopExits/undef-phis.ll
@@ -0,0 +1,58 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt < %s -passes='unify-loop-exits' -S | FileCheck %s
+
+define fastcc void @undef_phi(i64 %i5247, i1 %i4530, i1 %i4936.not) {
+; CHECK-LABEL: define fastcc void @undef_phi(
+; CHECK-SAME: i64 [[I5247:%.*]], i1 [[I4530:%.*]], i1 [[I4936_NOT:%.*]]) {
+; CHECK-NEXT:  [[BB:.*:]]
+; CHECK-NEXT:    br label %[[MBB3932:.*]]
+; CHECK:       [[MBB3932]]:
+; CHECK-NEXT:    br label %[[MBB4454:.*]]
+; CHECK:       [[MBB4321:.*]]:
+; CHECK-NEXT:    [[TMP0:%.*]] = trunc i64 [[I5247]] to i32
+; CHECK-NEXT:    [[I5290:%.*]] = icmp eq i32 [[TMP0]], 0
+; CHECK-NEXT:    br i1 [[I5290]], label %[[MBB3932]], label %[[LOOP_EXIT_GUARD:.*]]
+; CHECK:       [[MBB4454]]:
+; CHECK-NEXT:    br i1 [[I4530]], label %[[MBB4535:.*]], label %[[LOOP_EXIT_GUARD1:.*]]
+; CHECK:       [[MBB4531:.*]]:
+; CHECK-NEXT:    ret void
+; CHECK:       [[MBB4535]]:
+; CHECK-NEXT:    br i1 [[I4936_NOT]], label %[[LOOP_EXIT_GUARD1]], label %[[MBB4454]]
+; CHECK:       [[MBB5291:.*]]:
+; CHECK-NEXT:    [[I5293:%.*]] = insertvalue [2 x i32] zeroinitializer, i32 [[DOTMOVED:%.*]], 1
+; CHECK-NEXT:    store volatile [2 x i32] [[I5293]], ptr addrspace(5) null, align 4
+; CHECK-NEXT:    ret void
+; CHECK:       [[LOOP_EXIT_GUARD]]:
+; CHECK-NEXT:    [[DOTMOVED]] = phi i32 [ [[TMP0]], %[[MBB4321]] ], [ undef, %[[LOOP_EXIT_GUARD1]] ]
+; CHECK-NEXT:    [[GUARD_MBB4531:%.*]] = phi i1 [ false, %[[MBB4321]] ], [ [[GUARD_MBB4531_MOVED:%.*]], %[[LOOP_EXIT_GUARD1]] ]
+; CHECK-NEXT:    br i1 [[GUARD_MBB4531]], label %[[MBB4531]], label %[[MBB5291]]
+; CHECK:       [[LOOP_EXIT_GUARD1]]:
+; CHECK-NEXT:    [[GUARD_MBB4531_MOVED]] = phi i1 [ true, %[[MBB4454]] ], [ undef, %[[MBB4535]] ]
+; CHECK-NEXT:    [[GUARD_LOOP_EXIT_GUARD:%.*]] = phi i1 [ true, %[[MBB4454]] ], [ false, %[[MBB4535]] ]
+; CHECK-NEXT:    br i1 [[GUARD_LOOP_EXIT_GUARD]], label %[[LOOP_EXIT_GUARD]], label %[[MBB4321]]
+;
+mbb:
+  br label %mbb3932
+
+mbb3932:                                           ; preds = %mbb4321, %mbb
+  br label %mbb4454
+
+mbb4321:                                           ; preds = %mbb4535
+  %0 = trunc i64 %i5247 to i32
+  %i5290 = icmp eq i32 %0, 0
+  br i1 %i5290, label %mbb3932, label %mbb5291
+
+mbb4454:                                           ; preds = %mbb4535, %mbb3932
+  br i1 %i4530, label %mbb4535, label %mbb4531
+
+mbb4531:                                           ; preds = %mbb4454
+  ret void
+
+mbb4535:                                           ; preds = %mbb4454
+  br i1 %i4936.not, label %mbb4321, label %mbb4454
+
+mbb5291:                                           ; preds = %mbb4321
+  %i5293 = insertvalue [2 x i32] zeroinitializer, i32 %0, 1
+  store volatile [2 x i32] %i5293, ptr addrspace(5) null, align 4
+  ret void
+}

[arsenm]

Should this try to preserve poison?

[jdoerfert]

As of right now, this emits undef by default. We likely want to change both UndefValue::get to poison. Should I include that and track undef/poison separately?

[arsenm]

It probably makes little difference, can switch these to poison as a second step?

[jdoerfert]

OK, let's do this first, then we switch to poison.

[ssahasra]

I didn't really understand. Are we agreeing that it's okay to temporarily ignore UndefValue that are actually poison in this patch?

[arsenm]

All poisons are undef. Not all undefs are poison. It's safe, though suboptimal, to replace poison with undef

[AZero13]

Does this affect codegen correctness or is this an enhancement? If it is the former, should this be backported to 19.x?

[jdoerfert]

Does this affect codegen correctness or is this an enhancement? If it is the former, should this be backported to 19.x?

Didn't see this. It's just an enhancement. No functional change.