[InstCombine] Allow freezing multiple out-of-loop values #155638

c-rhodes · 2025-08-27T15:27:24Z

Extend foldFreezeIntoRecurrence to allow freezing multiple out-of-loop values. This is following on from #154336, which recently made the same change for a wider set of ops.

Extend foldFreezeIntoRecurrence to allow freezing multiple out-of-loop values. This is following on from llvm#154336, which recently made the same change for a wider set of ops.

llvmbot · 2025-08-27T15:28:00Z

@llvm/pr-subscribers-llvm-transforms

Author: Cullen Rhodes (c-rhodes)

Changes

Extend foldFreezeIntoRecurrence to allow freezing multiple out-of-loop values. This is following on from #154336, which recently made the same change for a wider set of ops.

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

2 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstructionCombining.cpp (+14-16)
(modified) llvm/test/Transforms/InstCombine/freeze.ll (+4-3)

diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 1c512ec1e21bb..e524839e2876e 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -5032,32 +5032,28 @@ Instruction *InstCombinerImpl::foldFreezeIntoRecurrence(FreezeInst &FI,
   // backedge values (possibly dropping poison flags along the way) until we
   // reach the phi again. In that case, we can move the freeze to the start
   // value.
-  Use *StartU = nullptr;
+  SmallVector<Use *> StartUses;
   SmallVector<Value *> Worklist;
   for (Use &U : PN->incoming_values()) {
-    if (DT.dominates(PN->getParent(), PN->getIncomingBlock(U))) {
+    BasicBlock *StartBB = PN->getIncomingBlock(U);
+    Value *StartV = U.get();
+    if (DT.dominates(PN->getParent(), StartBB)) {
       // Add backedge value to worklist.
-      Worklist.push_back(U.get());
+      Worklist.push_back(StartV);
       continue;
     }
 
-    // Don't bother handling multiple start values.
-    if (StartU)
+    bool StartNeedsFreeze = !isGuaranteedNotToBeUndefOrPoison(StartV);
+    // We can't insert freeze if a start value is the result of the
+    // terminator (e.g. an invoke).
+    if (StartNeedsFreeze && StartBB->getTerminator() == StartV)
       return nullptr;
-    StartU = &U;
+    StartUses.push_back(&U);
   }
 
-  if (!StartU || Worklist.empty())
+  if (StartUses.empty() || Worklist.empty())
     return nullptr; // Not a recurrence.
 
-  Value *StartV = StartU->get();
-  BasicBlock *StartBB = PN->getIncomingBlock(*StartU);
-  bool StartNeedsFreeze = !isGuaranteedNotToBeUndefOrPoison(StartV);
-  // We can't insert freeze if the start value is the result of the
-  // terminator (e.g. an invoke).
-  if (StartNeedsFreeze && StartBB->getTerminator() == StartV)
-    return nullptr;
-
   SmallPtrSet<Value *, 32> Visited;
   SmallVector<Instruction *> DropFlags;
   while (!Worklist.empty()) {
@@ -5084,7 +5080,9 @@ Instruction *InstCombinerImpl::foldFreezeIntoRecurrence(FreezeInst &FI,
   for (Instruction *I : DropFlags)
     I->dropPoisonGeneratingAnnotations();
 
-  if (StartNeedsFreeze) {
+  for (Use *StartU : StartUses) {
+    Value *StartV = StartU->get();
+    BasicBlock *StartBB = PN->getIncomingBlock(*StartU);
     Builder.SetInsertPoint(StartBB->getTerminator());
     Value *FrozenStartV = Builder.CreateFreeze(StartV,
                                                StartV->getName() + ".fr");
diff --git a/llvm/test/Transforms/InstCombine/freeze.ll b/llvm/test/Transforms/InstCombine/freeze.ll
index b29421a655fa8..09fb2dc19912c 100644
--- a/llvm/test/Transforms/InstCombine/freeze.ll
+++ b/llvm/test/Transforms/InstCombine/freeze.ll
@@ -1106,13 +1106,14 @@ define void @fold_phi_multiple_start_values(i1 %c, i32 %init, i32 %init2, i32 %n
 ; CHECK-LABEL: define void @fold_phi_multiple_start_values(
 ; CHECK-SAME: i1 [[C:%.*]], i32 [[INIT:%.*]], i32 [[INIT2:%.*]], i32 [[N:%.*]]) {
 ; CHECK-NEXT:  [[ENTRY:.*]]:
+; CHECK-NEXT:    [[INIT_FR:%.*]] = freeze i32 [[INIT]]
 ; CHECK-NEXT:    br i1 [[C]], label %[[IF:.*]], label %[[LOOP:.*]]
 ; CHECK:       [[IF]]:
+; CHECK-NEXT:    [[INIT2_FR:%.*]] = freeze i32 [[INIT2]]
 ; CHECK-NEXT:    br label %[[LOOP]]
 ; CHECK:       [[LOOP]]:
-; CHECK-NEXT:    [[I:%.*]] = phi i32 [ [[INIT]], %[[ENTRY]] ], [ [[INIT2]], %[[IF]] ], [ [[I_NEXT:%.*]], %[[LOOP]] ]
-; CHECK-NEXT:    [[I_FR:%.*]] = freeze i32 [[I]]
-; CHECK-NEXT:    [[I_NEXT]] = add nuw nsw i32 [[I_FR]], 1
+; CHECK-NEXT:    [[I:%.*]] = phi i32 [ [[INIT_FR]], %[[ENTRY]] ], [ [[INIT2_FR]], %[[IF]] ], [ [[I_NEXT:%.*]], %[[LOOP]] ]
+; CHECK-NEXT:    [[I_NEXT]] = add i32 [[I]], 1
 ; CHECK-NEXT:    [[COND:%.*]] = icmp eq i32 [[I_NEXT]], [[N]]
 ; CHECK-NEXT:    br i1 [[COND]], label %[[LOOP]], label %[[EXIT:.*]]
 ; CHECK:       [[EXIT]]:

c-rhodes · 2025-08-27T15:30:29Z

@nikic I had a look at implementing this for foldFreezeIntoRecurrence as you mentioned on #154336 (comment). There's an existing test for this, with this patch we'd be able to remove freeze altogether here: https://godbolt.org/z/8qsf3zcWG

nikic

Can you please add a test where the two start values come from the same predecessor (e.g. dummy br with both successors equal)? You need to be careful about that because the phi needs to have identical values for the same predecessor.

By the way, this change isn't actually what I had in mind... I'd expect multiple starting values to get canonicalized away by loop simplify form. I think the more interesting extension here is the case where you need to freeze both the start value and the step value. So something like {start,+,step} where both values are not known non-poison.

c-rhodes · 2025-08-28T14:22:29Z

Can you please add a test where the two start values come from the same predecessor (e.g. dummy br with both successors equal)? You need to be careful about that because the phi needs to have identical values for the same predecessor.

Perhaps I'm misunderstanding, but that's not legal IR? https://godbolt.org/z/8MYM1Yf5b

By the way, this change isn't actually what I had in mind... I'd expect multiple starting values to get canonicalized away by loop simplify form. I think the more interesting extension here is the case where you need to freeze both the start value and the step value. So something like {start,+,step} where both values are not known non-poison.

ah ok, do you have an example test for this? Sounds like

llvm-project/llvm/test/Transforms/InstCombine/freeze.ll

Line 855 in 33b2c26

define void @fold_phi_drop_flags(i32 %init, i32 %n) {

but the flags get dropped there

c-rhodes · 2025-09-09T10:00:16Z

Can you please add a test where the two start values come from the same predecessor (e.g. dummy br with both successors equal)? You need to be careful about that because the phi needs to have identical values for the same predecessor.

Perhaps I'm misunderstanding, but that's not legal IR? https://godbolt.org/z/8MYM1Yf5b

Apologies I think I understand your concern now. In this example: https://godbolt.org/z/W5e3rfx1E we want to avoid:

define void @fold_phi_multiple_identical_start_values(i1 %c, i32 %init, i32 %n) {
entry:
  %init.fr.0 = freeze i32 %init
  %init.fr.1 = freeze i32 %init
  br i1 %c, label %loop, label %loop

loop:
  %i = phi i32 [ %init.fr.0, %entry ], [ %init.fr.1, %entry ], [ %i.next, %loop ]
  %i.next = add nsw nuw i32 %i, 1
  %cond = icmp eq i32 %i.next, %n
  br i1 %cond, label %loop, label %exit

exit:
  ret void
}

the final IR is correct with %init.fr for both %entry incoming values for this.

By the way, this change isn't actually what I had in mind... I'd expect multiple starting values to get canonicalized away by loop simplify form.

You're right, if the multiple starting values are well-defined the freeze can be removed with a combination of loop-simplify and instcombine: https://godbolt.org/z/dh7jq5qov

I think the more interesting extension here is the case where you need to freeze both the start value and the step value. So something like {start,+,step} where both values are not known non-poison.

Seems this is already handled: https://godbolt.org/z/9oa4YvnYz

Not sure this patch has any value. It also doesn't help the test I added in #157112 either as that's not a recurrence.

nikic · 2025-09-09T13:13:35Z

Can you please add a test where the two start values come from the same predecessor (e.g. dummy br with both successors equal)? You need to be careful about that because the phi needs to have identical values for the same predecessor.

Perhaps I'm misunderstanding, but that's not legal IR? https://godbolt.org/z/8MYM1Yf5b

Apologies I think I understand your concern now. In this example: https://godbolt.org/z/W5e3rfx1E we want to avoid:
define void @fold_phi_multiple_identical_start_values(i1 %c, i32 %init, i32 %n) {
entry:
  %init.fr.0 = freeze i32 %init
  %init.fr.1 = freeze i32 %init
  br i1 %c, label %loop, label %loop

loop:
  %i = phi i32 [ %init.fr.0, %entry ], [ %init.fr.1, %entry ], [ %i.next, %loop ]
  %i.next = add nsw nuw i32 %i, 1
  %cond = icmp eq i32 %i.next, %n
  br i1 %cond, label %loop, label %exit

exit:
  ret void
}
the final IR is correct with %init.fr for both %entry incoming values for this.

Yes, exactly. I think it ends up being correct by accident because we later CSE the two freezes. You may see intermediate invalid IR using -debug-counter.

By the way, this change isn't actually what I had in mind... I'd expect multiple starting values to get canonicalized away by loop simplify form.

You're right, if the multiple starting values are well-defined the freeze can be removed with a combination of loop-simplify and instcombine: https://godbolt.org/z/dh7jq5qov

I think the more interesting extension here is the case where you need to freeze both the start value and the step value. So something like {start,+,step} where both values are not known non-poison.

Seems this is already handled: https://godbolt.org/z/9oa4YvnYz

The case I had in mind is this one: https://godbolt.org/z/cczGhW8K9 Where we'd freeze both init and step.

c-rhodes · 2025-09-09T14:03:29Z

Can you please add a test where the two start values come from the same predecessor (e.g. dummy br with both successors equal)? You need to be careful about that because the phi needs to have identical values for the same predecessor.

Perhaps I'm misunderstanding, but that's not legal IR? https://godbolt.org/z/8MYM1Yf5b

Apologies I think I understand your concern now. In this example: https://godbolt.org/z/W5e3rfx1E we want to avoid:
define void @fold_phi_multiple_identical_start_values(i1 %c, i32 %init, i32 %n) {
entry:
  %init.fr.0 = freeze i32 %init
  %init.fr.1 = freeze i32 %init
  br i1 %c, label %loop, label %loop

loop:
  %i = phi i32 [ %init.fr.0, %entry ], [ %init.fr.1, %entry ], [ %i.next, %loop ]
  %i.next = add nsw nuw i32 %i, 1
  %cond = icmp eq i32 %i.next, %n
  br i1 %cond, label %loop, label %exit

exit:
  ret void
}
the final IR is correct with %init.fr for both %entry incoming values for this.
Yes, exactly. I think it ends up being correct by accident because we later CSE the two freezes. You may see intermediate invalid IR using -debug-counter.

Ah yes, you're right:

build/bin/opt -passes=instcombine -o - fold_phi_multiple_start_values.ll -S -debug-counter=instcombine-visit=1-2
PHI node has multiple entries for the same basic block with different incoming values!
  %i = phi i32 [ %init.fr, %entry ], [ %init.fr1, %entry ], [ %i.next, %loop ]
label %entry
  %init.fr1 = freeze i32 %init
  %init.fr = freeze i32 %init
LLVM ERROR: Broken module found, compilation aborted!
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace and instructions to reproduce the bug.
Stack dump:
0.      Program arguments: build/bin/opt -passes=instcombine -o - fold_phi_multiple_start_values.ll -S -debug-counter=instcombine-visit=1-2
1.      Running pass "verify" on module "fold_phi_multiple_start_values.ll"
 #0 0x0000e06917fc5098 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/culrho01/llvm-project/build/bin/../lib/libLLVMSupport.so.22.0git+0x245098)
 #1 0x0000e06917fc2a28 llvm::sys::RunSignalHandlers() (/home/culrho01/llvm-project/build/bin/../lib/libLLVMSupport.so.22.0git+0x242a28)
 #2 0x0000e06917fc5ea8 SignalHandler(int, siginfo_t*, void*) Signals.cpp:0:0
 #3 0x0000e0691c325968 (linux-vdso.so.1+0x968)
 #4 0x0000e069178c7608 __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #5 0x0000e0691787cb3c raise ./signal/../sysdeps/posix/raise.c:27:6
 #6 0x0000e06917867e00 abort ./stdlib/abort.c:81:7
 #7 0x0000e06917effab0 llvm::report_fatal_error(llvm::Twine const&, bool) (/home/culrho01/llvm-project/build/bin/../lib/libLLVMSupport.so.22.0git+0x17fab0)
 #8 0x0000e06917eff8e4 llvm::report_fatal_error(llvm::Twine const&, bool) (/home/culrho01/llvm-project/build/bin/../lib/libLLVMSupport.so.22.0git+0x17f8e4)
 #9 0x0000e0691869b8b8 llvm::VerifierPass::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (/home/culrho01/llvm-project/build/bin/../lib/libLLVMCore.so.22.0git+0x53b8b8)
#10 0x0000e0691865caa8 llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/home/culrho01/llvm-project/build/bin/../lib/libLLVMCore.so.22.0git+0x4fcaa8)
#11 0x0000e0691c2a7128 llvm::runPassPipeline(llvm::StringRef, llvm::Module&, llvm::TargetMachine*, llvm::TargetLibraryInfoImpl*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::StringRef, llvm::ArrayRef<llvm::PassPlugin>, llvm::ArrayRef<std::function<void (llvm::PassBuilder&)>>, llvm::opt_tool::OutputKind, llvm::opt_tool::VerifierKind, bool, bool, bool, bool, bool, bool, bool, bool) (/home/culrho01/llvm-project/build/bin/../lib/libLLVMOptDriver.so.22.0git+0x37128)
#12 0x0000e0691c2b5dd8 optMain (/home/culrho01/llvm-project/build/bin/../lib/libLLVMOptDriver.so.22.0git+0x45dd8)
#13 0x0000e069178684c4 __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:74:3
#14 0x0000e06917868598 call_init ./csu/../csu/libc-start.c:128:20
#15 0x0000e06917868598 __libc_start_main ./csu/../csu/libc-start.c:347:5
#16 0x0000b0b3c1d10bf0 _start (build/bin/opt+0x10bf0)
[1]    1100609 abort (core dumped)  build/bin/opt -passes=instcombine -o - fold_phi_multiple_start_values.ll -S

I wasn't aware of that option, thanks!

By the way, this change isn't actually what I had in mind... I'd expect multiple starting values to get canonicalized away by loop simplify form.

You're right, if the multiple starting values are well-defined the freeze can be removed with a combination of loop-simplify and instcombine: https://godbolt.org/z/dh7jq5qov

I think the more interesting extension here is the case where you need to freeze both the start value and the step value. So something like {start,+,step} where both values are not known non-poison.

Seems this is already handled: https://godbolt.org/z/9oa4YvnYz

The case I had in mind is this one: https://godbolt.org/z/cczGhW8K9 Where we'd freeze both init and step.

Ah ok thanks, will see if it can handle that then.

I have been looking at a fix for #157112 separately and have a small change to support non-recurrence PHIs in pushFreezeToPreventPoisonFromPropagating which fixes it, but I wouldn't be surprised if there's some edge case I've not considered. I'll create a PR for that.

[InstCombine] Allow freezing multiple out-of-loop values

f330a28

Extend foldFreezeIntoRecurrence to allow freezing multiple out-of-loop values. This is following on from llvm#154336, which recently made the same change for a wider set of ops.

c-rhodes requested a review from dtcxzyw August 27, 2025 15:27

c-rhodes requested a review from nikic as a code owner August 27, 2025 15:27

llvmbot added llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms labels Aug 27, 2025

nikic reviewed Aug 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[InstCombine] Allow freezing multiple out-of-loop values #155638

[InstCombine] Allow freezing multiple out-of-loop values #155638

Uh oh!

c-rhodes commented Aug 27, 2025

Uh oh!

llvmbot commented Aug 27, 2025

Uh oh!

c-rhodes commented Aug 27, 2025

Uh oh!

nikic left a comment

Uh oh!

c-rhodes commented Aug 28, 2025

Uh oh!

c-rhodes commented Sep 9, 2025

Uh oh!

nikic commented Sep 9, 2025

Uh oh!

c-rhodes commented Sep 9, 2025

Uh oh!

Uh oh!

[InstCombine] Allow freezing multiple out-of-loop values #155638

Are you sure you want to change the base?

[InstCombine] Allow freezing multiple out-of-loop values #155638

Uh oh!

Conversation

c-rhodes commented Aug 27, 2025

Uh oh!

llvmbot commented Aug 27, 2025

Uh oh!

c-rhodes commented Aug 27, 2025

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

c-rhodes commented Aug 28, 2025

Uh oh!

c-rhodes commented Sep 9, 2025

Uh oh!

nikic commented Sep 9, 2025

Uh oh!

c-rhodes commented Sep 9, 2025

Uh oh!

Uh oh!