[llvm] [InstCombine] Reuse existing freeze when pushing freeze through a binop (PR #202306)

[via llvm-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-transforms

Author: Yuyang Zhang (yuyzhang512)

<details>
<summary>Changes</summary>

pushFreezeToPreventPoisonFromPropagating bailed out of `freeze(binop a b)` when both operands were maybe-poison. freezeOtherUses would then migrate one operand to an existing freeze on a later iteration, and only then would this transform fire -- a two-step convergence that tripped the InstCombine fixpoint verifier.

Extend the two-operand path: look for an existing FreezeInst on either operand, hoisting it past its operand's def to dominate when needed (mirroring freezeOtherUses' move logic). If found, substitute and fall through to the original single-maybe-poison-operand path. No extra freezes when no existing one is available -- preserves the original bail.

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


2 Files Affected:

- (modified) llvm/lib/Transforms/InstCombine/InstructionCombining.cpp (+55-4) 
- (modified) llvm/test/Transforms/InstCombine/freeze.ll (+25) 


``````````diff
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 89bf4bf713421..976262849d5ff 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -5257,16 +5257,67 @@ InstCombinerImpl::pushFreezeToPreventPoisonFromPropagating(FreezeInst &OrigFI) {
   // If operand is guaranteed not to be poison, there is no need to add freeze
   // to the operand. So we first find the operand that is not guaranteed to be
   // poison.
+  //
+  // Helper: when more than one operand is maybe-poison, look for an existing
+  // freeze of that operand anywhere in the function and substitute it,
+  // hoisting it to dominate `OrigOpInst` if needed.  This avoids a fixpoint
+  // failure where `freezeOtherUses` would migrate the alias-replacement on a
+  // later iteration -- and only THEN would this transform become applicable
+  // (a single maybe-poison operand remaining).  Doing the substitution here
+  // lets the rewrite converge in a single InstCombine iteration without
+  // creating extra freezes.
+  auto reuseExistingFreezeFor = [&](Value *V) -> Value * {
+    for (User *U : V->users()) {
+      auto *FI = dyn_cast<FreezeInst>(U);
+      if (!FI || FI == &OrigFI)
+        continue;
+      if (DT.dominates(FI, OrigOpInst))
+        return FI;
+      // Hoist the freeze to immediately after its operand's def so it
+      // dominates `OrigOpInst`.  Mirror `freezeOtherUses`' move logic.
+      BasicBlock::iterator MoveBefore;
+      if (isa<Argument>(V)) {
+        MoveBefore = OrigOpInst->getFunction()
+                         ->getEntryBlock()
+                         .getFirstNonPHIOrDbgOrAlloca();
+      } else {
+        auto Opt = cast<Instruction>(V)->getInsertionPointAfterDef();
+        if (!Opt)
+          continue;
+        MoveBefore = *Opt;
+      }
+      MoveBefore.setHeadBit(false);
+      if (FI != &*MoveBefore)
+        FI->moveBefore(*MoveBefore->getParent(), MoveBefore);
+      if (DT.dominates(FI, OrigOpInst))
+        return FI;
+    }
+    return nullptr;
+  };
+
   Value *MaybePoisonOperand = nullptr;
-  for (Value *V : OrigOpInst->operands()) {
+  for (Use &U : OrigOpInst->operands()) {
+    Value *V = U.get();
     if (isa<MetadataAsValue>(V) || isGuaranteedNotToBeUndefOrPoison(V) ||
         // Treat identical operands as a single operand.
         (MaybePoisonOperand && MaybePoisonOperand == V))
       continue;
-    if (!MaybePoisonOperand)
+    if (!MaybePoisonOperand) {
       MaybePoisonOperand = V;
-    else
-      return nullptr;
+      continue;
+    }
+    // Two distinct maybe-poison operands.  Try to reuse an existing freeze
+    // on one of them to collapse back to a single maybe-poison operand.
+    if (Value *Existing = reuseExistingFreezeFor(V)) {
+      U.set(Existing);
+      continue;
+    }
+    if (Value *Existing = reuseExistingFreezeFor(MaybePoisonOperand)) {
+      OrigOpInst->replaceUsesOfWith(MaybePoisonOperand, Existing);
+      MaybePoisonOperand = V;
+      continue;
+    }
+    return nullptr;
   }
 
   OrigOpInst->dropPoisonGeneratingAnnotations();
diff --git a/llvm/test/Transforms/InstCombine/freeze.ll b/llvm/test/Transforms/InstCombine/freeze.ll
index 3a401acb6d3ee..1c9f94d7ff735 100644
--- a/llvm/test/Transforms/InstCombine/freeze.ll
+++ b/llvm/test/Transforms/InstCombine/freeze.ll
@@ -1733,6 +1733,31 @@ define float @freeze_fabs_nofpclass(float %a) {
   ret float %x.fr
 }
 
+; A single-use binop has two maybe-poison operands, but one of them
+; (%b) already has a freeze in the function (%fb).  The freeze-into-binop
+; transform reuses %fb (hoisting it past %b's def in the entry block so
+; it dominates %m), substitutes it in %m, and falls through to the
+; single-maybe-poison-operand path which freezes %a and removes %fm.
+; This must converge in one InstCombine iteration (regression test for
+; the freeze-binop fixpoint failure).
+define i1 @reuse_existing_freeze_binop(i32 %a, i32 %b) {
+; CHECK-LABEL: define i1 @reuse_existing_freeze_binop(
+; CHECK-SAME: i32 [[A:%.*]], i32 [[B:%.*]]) {
+; CHECK-NEXT:    [[FB:%.*]] = freeze i32 [[B]]
+; CHECK-NEXT:    [[A_FR:%.*]] = freeze i32 [[A]]
+; CHECK-NEXT:    [[M:%.*]] = mul i32 [[A_FR]], [[FB]]
+; CHECK-NEXT:    [[P:%.*]] = mul i32 [[M]], [[FB]]
+; CHECK-NEXT:    [[R:%.*]] = icmp eq i32 [[P]], 0
+; CHECK-NEXT:    ret i1 [[R]]
+;
+  %m  = mul i32 %a, %b
+  %fm = freeze i32 %m
+  %fb = freeze i32 %b
+  %p  = mul i32 %fm, %fb
+  %r  = icmp eq i32 %p, 0
+  ret i1 %r
+}
+
 !0 = !{}
 !1 = !{i64 4}
 !2 = !{i32 0, i32 100}

``````````

</details>


https://github.com/llvm/llvm-project/pull/202306