[InstCombine] Reuse existing freeze when pushing freeze through a binop#202306
Open
yuyzhang512 wants to merge 1 commit into
Open
[InstCombine] Reuse existing freeze when pushing freeze through a binop#202306yuyzhang512 wants to merge 1 commit into
yuyzhang512 wants to merge 1 commit into
Conversation
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.
Author
|
Alive2 proof: https://alive2.llvm.org/ce/z/YjzGSK |
|
Hello @yuyzhang512 👋 Thank you for submitting a Pull Request (PR) to the LLVM Project. Since this is your first PR, here are a few useful links covering our main contribution policies and review practices.
Please reply to this message to confirm that you have read these policies, especially the LLVM AI Tool Use Policy, and that any AI tool usage has been noted in the PR description. Frequently asked questionsHow do I add reviewers? This PR will be automatically labeled, and the relevant teams will be notified. For some parts of the project, reviewers may also be added automatically. You can also add reviewers manually using the Reviewers section on this page. If you cannot use that section, it is probably because you do not have write permissions for the repository. In that case, you can request a review by tagging reviewers in a comment using What if there are no comments? If you have not received any comments on your PR after a week, you can request a review by pinging the PR with a comment such as “Ping”. The common courtesy ping rate is once a week. Please remember that you are asking for volunteer time from other developers. Are any special GitHub settings required to contribute to LLVM? We only require contributors to have a public email address associated with their GitHub commits, see this section of LLVM Developer Policy for details. If you have questions, feel free to leave a comment on this PR, or ask on LLVM Discord or LLVM Discourse. Thank you, |
llvmorg-github-actions
Bot
added
llvm:instcombine
|
@llvm/pr-subscribers-llvm-transforms Author: Yuyang Zhang (yuyzhang512) ChangespushFreezeToPreventPoisonFromPropagating bailed out of 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:
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}
|
Member
|
The following correctness issue was found by llvm-hackme. This comment is generated by an automated correctness checking service designed to help identify critical correctness bugs (opt crashes or Alive2 miscompilations) and improve PR review efficiency under limited reviewer bandwidth. ReproducerKind: crash IR Reproducer: ; RUN: opt -passes=instcombine<no-verify-fixpoint> -S
define {i32} @test(i32 %a) {
%iv = insertvalue {i32} undef, i32 %a, 0
%fr = freeze {i32} %iv
ret {i32} %fr
}Stacktrace: Baseline Revision: |
dtcxzyw
requested changes
Jun 8, 2026
dtcxzyw
left a comment
dtcxzyw
left a comment
Member
There was a problem hiding this comment.
A correctness issue was identified by llvm-hackme. Please see the issue comment for details.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
pushFreezeToPreventPoisonFromPropagatingbailed out offreeze(binop a b)when both operands were maybe-poison.freezeOtherUseswould 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 on e.g. AMDGPU MoE GEMM kernels that load a tile-stride scalar as<1 x i32>, extract, multiply, and use the result as a divisor.Minimal repro:
Extend the two-operand path: when we hit a second distinct maybe-poison operand, look for an existing
FreezeInston either operand and substitute it (hoisting past its operand's def to dominate when needed — mirrorsfreezeOtherUses' move logic). If found, fall through to the original single-maybe-poison-operand path. No extra freezes when no existing one is available — preserves the original conservative bail.Correctness: the underlying "push freeze into a non-poison-creating one-use binop" transform is already in mainline. Substituting an operand with
freeze operandis a refinement (narrows the value set when the operand is poison, no-op otherwise). Composition of refinements is a refinement.Regression test added in
freeze.ll(@reuse_existing_freeze_binop). FullTransforms/InstCombine/lit suite passes (1809 tests).