[llvm] PeepholeOpt: Avoid infinite loop on cyclic rewrite maps (PR #216271)
Jameson Nash via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 14 01:16:17 PDT 2026
https://github.com/vtjnash created https://github.com/llvm/llvm-project/pull/216271
findNextSource() records every use-def step it takes in RewriteMap, and getNewSource() later walks that map, recursing into the sources of each PHI entry. The walk assumes the map is acyclic, but findNextSource() did not guarantee that: a chain is not looked up in the map when it ends on a suitable source, and a chain ending on an existing single-source entry assumes that entry leads to an already resolved source. Either way the chain may lead back into a PHI that is part of the same traversal, leaving a cycle in the map that getNewSource() then follows in infinite recursion until it overflows the stack.
The existing guard at the try_emplace site only catches a pair that is itself re-tracked as a multi-source entry; it cannot see a cycle that re-enters the visited region at a single-source key upstream of the PHI. The trigger is a pair of cross register bank bitcasts around a loop carried PHI, e.g. on x86-64:
bb.1:
%1:gr64 = PHI %0, %bb.0, %3, %bb.3
bb.3:
%2:fr64 = MOV64toSDrr %1
%3:gr64 = MOVSDto64rr %2
Tracking %1 from the MOV64toSDrr builds {%2 -> %1, %1 -> PHI(%0, %3), %3 -> %2}: the %3 chain terminates on %2, a suitable fr64 source, without ever being checked against the map.
Fix this by verifying, before findNextSource() reports success, that the part of the map reachable from the queried pair is acyclic, and aborting the rewrite otherwise. The check runs before anything is mutated, which keeps optimizeUncoalescableCopy()'s check-everything-then-commit structure intact. Since only PHI entries can make a cycle, the check is skipped when no PHI was traversed. It cannot be skipped based on this call's traversal alone, though: callers may share one map across several defs of the same instruction, and a later PHI-free chain can link up with an earlier call's entries to form a cycle, so the check also runs whenever the map already had entries on entry.
Legitimate rewrites are unaffected: the check rejects only maps on which getNewSource() would not have terminated. On the example above the MOV64toSDrr keeps its operand while the MOVSDto64rr is still rewritten to a COPY of the PHI.
RewritePHILimit does not help here: the recursion is a true cycle, so any limit permitting more than one PHI still crashes, and a limit of one disables PHI look-through entirely.
Originally reported in 2018; rediscovered from JuliaLang/julia#50408.
Fixes #36621
---
Disclaimer: almost everything was written by Claude Fable in the end, since it identified issues with the proposals given previously in the issue and also all the proposals given by Opus (either in trying to detect the issue later, being more point-wise with the detection, or cap the walk better with the existing limit). So in the end, my only real input was in trying to get it to write better comments, but I apologize that they are likely still a bit awkward.
>From 5342ab6dc3f8a452c12c8fe2b192797b320489cd Mon Sep 17 00:00:00 2001
From: Jameson Nash <vtjnash at gmail.com>
Date: Fri, 14 Aug 2026 06:55:25 +0000
Subject: [PATCH] PeepholeOpt: Avoid infinite loop on cyclic rewrite maps
findNextSource() records every use-def step it takes in RewriteMap, and
getNewSource() later walks that map, recursing into the sources of each
PHI entry. The walk assumes the map is acyclic, but findNextSource() did
not guarantee that: a chain is not looked up in the map when it ends on
a suitable source, and a chain ending on an existing single-source entry
assumes that entry leads to an already resolved source. Either way the
chain may lead back into a PHI that is part of the same traversal,
leaving a cycle in the map that getNewSource() then follows in infinite
recursion until it overflows the stack.
The existing guard at the try_emplace site only catches a pair that is
itself re-tracked as a multi-source entry; it cannot see a cycle that
re-enters the visited region at a single-source key upstream of the PHI.
The trigger is a pair of cross register bank bitcasts around a loop
carried PHI, e.g. on x86-64:
bb.1:
%1:gr64 = PHI %0, %bb.0, %3, %bb.3
bb.3:
%2:fr64 = MOV64toSDrr %1
%3:gr64 = MOVSDto64rr %2
Tracking %1 from the MOV64toSDrr builds {%2 -> %1, %1 -> PHI(%0, %3), %3
-> %2}: the %3 chain terminates on %2, a suitable fr64 source, without
ever being checked against the map.
Fix this by verifying, before findNextSource() reports success, that the
part of the map reachable from the queried pair is acyclic, and aborting
the rewrite otherwise. The check runs before anything is mutated, which
keeps optimizeUncoalescableCopy()'s check-everything-then-commit
structure intact. Since only PHI entries can make a cycle, the check is
skipped when no PHI was traversed. It cannot be skipped based on this
call's traversal alone, though: callers may share one map across several
defs of the same instruction, and a later PHI-free chain can link up
with an earlier call's entries to form a cycle, so the check also runs
whenever the map already had entries on entry.
Legitimate rewrites are unaffected: the check rejects only maps on which
getNewSource() would not have terminated. On the example above the
MOV64toSDrr keeps its operand while the MOVSDto64rr is still rewritten
to a COPY of the PHI.
RewritePHILimit does not help here: the recursion is a true cycle, so
any limit permitting more than one PHI still crashes, and a limit of one
disables PHI look-through entirely.
Originally reported in 2018; rediscovered from JuliaLang/julia#50408.
Fixes #36621
Co-Authored-By: Claude Opus 5 <noreply at anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply at anthropic.com>
---
llvm/lib/CodeGen/PeepholeOptimizer.cpp | 73 +++++++++++++++++++-
llvm/test/CodeGen/X86/peephole-phi-cycle.ll | 29 ++++++++
llvm/test/CodeGen/X86/peephole-phi-cycle.mir | 43 ++++++++++++
3 files changed, 144 insertions(+), 1 deletion(-)
create mode 100644 llvm/test/CodeGen/X86/peephole-phi-cycle.ll
create mode 100644 llvm/test/CodeGen/X86/peephole-phi-cycle.mir
diff --git a/llvm/lib/CodeGen/PeepholeOptimizer.cpp b/llvm/lib/CodeGen/PeepholeOptimizer.cpp
index d46ace0bdd748..e6dc8f345d782 100644
--- a/llvm/lib/CodeGen/PeepholeOptimizer.cpp
+++ b/llvm/lib/CodeGen/PeepholeOptimizer.cpp
@@ -67,6 +67,7 @@
#include "llvm/CodeGen/PeepholeOptimizer.h"
#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallSet.h"
#include "llvm/ADT/SmallVector.h"
@@ -997,6 +998,44 @@ bool PeepholeOptimizer::optimizeCondBranch(MachineInstr &MI) {
return TII->optimizeCondBranch(MI);
}
+/// Check whether the part of \p RewriteMap that is reachable from \p Def
+/// contains a cycle. getNewSource() walks that subgraph and would not
+/// terminate on a cyclic one.
+static bool hasRewriteCycle(RegSubRegPair Def,
+ const PeepholeOptimizer::RewriteMapTy &RewriteMap) {
+ // Iterative depth first search. Visited holds every pair that has been
+ // pushed on the stack, OnPath only those on the path currently being
+ // explored: reaching one of them again means there is a cycle.
+ SmallDenseSet<RegSubRegPair, 8> Visited, OnPath;
+ SmallVector<std::pair<RegSubRegPair, int>, 8> Stack;
+
+ Visited.insert(Def);
+ OnPath.insert(Def);
+ Stack.emplace_back(Def, 0);
+
+ while (!Stack.empty()) {
+ auto [Node, NextSrcIdx] = Stack.back();
+ auto It = RewriteMap.find(Node);
+ // Pairs that are not keys are the next sources, i.e. the leaves.
+ if (It == RewriteMap.end() || NextSrcIdx == It->second.getNumSources()) {
+ OnPath.erase(Node);
+ Stack.pop_back();
+ continue;
+ }
+
+ ++Stack.back().second;
+ RegSubRegPair Src = It->second.getSrc(NextSrcIdx);
+ if (OnPath.contains(Src))
+ return true;
+ if (Visited.insert(Src).second) {
+ OnPath.insert(Src);
+ Stack.emplace_back(Src, 0);
+ }
+ }
+
+ return false;
+}
+
/// Try to find a better source value that shares the same register file to
/// replace \p RegSubReg in an instruction like
/// `DefRC.DefSubReg = COPY RegSubReg`
@@ -1022,6 +1061,18 @@ bool PeepholeOptimizer::findNextSource(const TargetRegisterClass *DefRC,
RegSubRegPair CurSrcPair = RegSubReg;
SmallVector<RegSubRegPair, 4> SrcToLook = {CurSrcPair};
+ // Callers may share one RewriteMap across several defs of the same
+ // instruction; remember whether it already holds entries from an earlier
+ // call, as the chains built below can link up with them.
+ //
+ // Note the contract this imposes on such callers: a failed call leaves
+ // the entries it already inserted in the map (there is no rollback),
+ // including cycle-forming ones inserted before an early return that the
+ // cycle check below never ran on. A caller sharing a map must therefore
+ // abandon it entirely, rewriting nothing from it, as soon as any
+ // findNextSource call on it returns false.
+ const bool HadSharedEntries = !RewriteMap.empty();
+
unsigned PHICount = 0;
do {
CurSrcPair = SrcToLook.pop_back_val();
@@ -1096,7 +1147,22 @@ bool PeepholeOptimizer::findNextSource(const TargetRegisterClass *DefRC,
} while (!SrcToLook.empty());
// If we did not find a more suitable source, there is nothing to optimize.
- return CurSrcPair.Reg != Reg;
+ if (CurSrcPair.Reg == Reg)
+ return false;
+
+ // RewriteMap may still contain a cycle: a chain ending on a suitable
+ // source or on an already visited entry is not followed any further, even
+ // when the entries from there lead back into a PHI of this same traversal
+ // (e.g. bitcasts of a loop carried PHI). getNewSource() would follow such
+ // a cycle forever. Only PHI entries can create a cycle, so the map only
+ // needs checking when a PHI was traversed or when it holds shared entries.
+ if ((PHICount > 0 || HadSharedEntries) &&
+ hasRewriteCycle(RegSubReg, RewriteMap)) {
+ LLVM_DEBUG(dbgs() << "findNextSource: RewriteMap is cyclic, aborting...\n");
+ return false;
+ }
+
+ return true;
}
/// Insert a PHI instruction with incoming edges \p SrcRegs that are
@@ -1373,6 +1439,11 @@ bool PeepholeOptimizer::optimizeUncoalescableCopy(
// If we do not know how to rewrite this definition, there is no point
// in trying to kill this instruction.
+ //
+ // This must give up on the whole instruction rather than skip this def:
+ // the failed call may have left entries in the shared RewriteMap,
+ // including cycle-forming ones its own cycle check never ran on, so the
+ // map must not be used for rewriting once any call has failed.
if (!findNextSource(DefRC, Def.SubReg, Def, RewriteMap))
return false;
diff --git a/llvm/test/CodeGen/X86/peephole-phi-cycle.ll b/llvm/test/CodeGen/X86/peephole-phi-cycle.ll
new file mode 100644
index 0000000000000..7f96869bb6b65
--- /dev/null
+++ b/llvm/test/CodeGen/X86/peephole-phi-cycle.ll
@@ -0,0 +1,29 @@
+; RUN: llc -mtriple=x86_64-- -o /dev/null %s
+
+; The bitcasts around the loop carried PHI used to make the rewrite map built
+; by the peephole optimizer cyclic, which sent getNewSource() into infinite
+; recursion (issue #36621). Check that this compiles at all.
+;
+; Note that the unused %sel and %cmp both look dead but are required to get
+; instruction selection to produce the cross register bank bitcast pattern
+; that triggered the bug.
+
+define void @phi_cycle(double %x, i1 %c) {
+top:
+ %i = bitcast double %x to i64
+ %sel = select i1 false, i64 %i, i64 0
+ br label %loop
+
+loop:
+ %phi = phi i64 [ %i, %top ], [ %back, %latch ]
+ %d = bitcast i64 %phi to double
+ br i1 %c, label %exit, label %latch
+
+exit:
+ unreachable
+
+latch:
+ %cmp = fcmp ule double 0.000000e+00, %d
+ %back = bitcast double %d to i64
+ br label %loop
+}
diff --git a/llvm/test/CodeGen/X86/peephole-phi-cycle.mir b/llvm/test/CodeGen/X86/peephole-phi-cycle.mir
new file mode 100644
index 0000000000000..e130ccceb667e
--- /dev/null
+++ b/llvm/test/CodeGen/X86/peephole-phi-cycle.mir
@@ -0,0 +1,43 @@
+# RUN: llc -mtriple=x86_64-- -run-pass=peephole-opt -verify-machineinstrs -o - %s | FileCheck %s
+
+# A pair of cross register bank bitcasts feeding a loop carried PHI makes the
+# rewrite map built by findNextSource() cyclic. Check that this is detected
+# instead of sending getNewSource() into infinite recursion (issue #36621).
+
+---
+name: phi_cycle
+tracksRegLiveness: true
+body: |
+ ; CHECK-LABEL: name: phi_cycle
+ bb.0:
+ successors: %bb.1(0x80000000)
+ liveins: $xmm0, $edi
+
+ %0:gr32 = COPY $edi
+ %1:fr64 = COPY $xmm0
+ %2:gr8 = COPY %0.sub_8bit
+ %3:gr64 = MOVSDto64rr %1
+
+ bb.1:
+ successors: %bb.2(0x00000000), %bb.3(0x80000000)
+
+ ; CHECK: %4:gr64 = PHI %3, %bb.0, [[BACK:%[0-9]+]], %bb.3
+ %4:gr64 = PHI %3, %bb.0, %5, %bb.3
+ TEST8ri %2, 1, implicit-def $eflags
+ JCC_1 %bb.3, 4, implicit $eflags
+ JMP_1 %bb.2
+
+ bb.2:
+ successors:
+
+ bb.3:
+ successors: %bb.1(0x80000000)
+
+ ; The value tracked from %6 leads back to the PHI, so this one is left
+ ; alone, but %5 is still rewritten into a copy of the PHI.
+ ; CHECK: %6:fr64 = MOV64toSDrr %4
+ ; CHECK-NEXT: [[BACK]]:gr64 = COPY %4
+ %6:fr64 = MOV64toSDrr %4
+ %5:gr64 = MOVSDto64rr %6
+ JMP_1 %bb.1
+...
More information about the llvm-commits
mailing list