[llvm-branch-commits] [llvm] [AMDGPU] Fix inconsistencies in RP tracking `advance`/`reset` behavior (PR #196099)
Lucas Ramirez via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Fri May 8 09:33:40 PDT 2026
https://github.com/lucas-rami updated https://github.com/llvm/llvm-project/pull/196099
>From b5abaa6a97827f041d493cf33b0ef4e0dbfc52f9 Mon Sep 17 00:00:00 2001
From: Lucas Ramirez <lucas.rami at proton.me>
Date: Wed, 6 May 2026 15:31:12 +0000
Subject: [PATCH 1/2] [AMDGPU] Fix inconsistencies in RP tracking
`advance`/`reset` behavior
Some of the variants of `advance` and `reset` in the `GCNRPTracker` and
`GCNDownwardRPTracker` had unclear/inconsistent semantics on their
return value. This aims to clarify that through improved documentation
and light functional changes.
These inconsistencies ultimately triggered an assert in
`GCNRPTaget::saveRP` on a complex kernel during scheduling.
`GCNScheduleDAGMILive::getRealRegPressure` would incorrectly return a
null pressure for a non-empty region which only had debug values. Such
regions can arise if the `PreRARematStage` rematerializes all non-debug
instructions out of their original region, leaving only debug values.
Attempting to rematerialize registers across that same region afterwards
would trigger the assert.
---
llvm/lib/Target/AMDGPU/GCNRegPressure.cpp | 10 +++--
llvm/lib/Target/AMDGPU/GCNRegPressure.h | 28 ++++++++++---
llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp | 12 ++++--
.../Target/AMDGPU/GCNRegPressureTest.cpp | 41 ++++++-------------
4 files changed, 49 insertions(+), 42 deletions(-)
diff --git a/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp b/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp
index 683e658aa4fb4..9b8e63a39fab8 100644
--- a/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp
+++ b/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp
@@ -621,13 +621,14 @@ void GCNUpwardRPTracker::recede(const MachineInstr &MI) {
// GCNDownwardRPTracker
bool GCNDownwardRPTracker::reset(const MachineInstr &MI,
- const LiveRegSet *LiveRegsCopy) {
+ MachineBasicBlock::const_iterator End,
+ const LiveRegSet *LiveRegsCopy) {
MRI = &MI.getMF()->getRegInfo();
LastTrackedMI = nullptr;
MBBEnd = MI.getParent()->end();
NextMI = &MI;
- NextMI = skipDebugInstructionsForward(NextMI, MBBEnd);
- if (NextMI == MBBEnd)
+ NextMI = skipDebugInstructionsForward(NextMI, End);
+ if (NextMI == End)
return false;
GCNRPTracker::reset(*NextMI, LiveRegsCopy, false);
return true;
@@ -746,7 +747,8 @@ bool GCNDownwardRPTracker::advance(MachineBasicBlock::const_iterator End) {
bool GCNDownwardRPTracker::advance(MachineBasicBlock::const_iterator Begin,
MachineBasicBlock::const_iterator End,
const LiveRegSet *LiveRegsCopy) {
- reset(*Begin, LiveRegsCopy);
+ if (!reset(*Begin, End, LiveRegsCopy))
+ return false;
return advance(End);
}
diff --git a/llvm/lib/Target/AMDGPU/GCNRegPressure.h b/llvm/lib/Target/AMDGPU/GCNRegPressure.h
index 00cb617a55fa7..97d2f5ad59278 100644
--- a/llvm/lib/Target/AMDGPU/GCNRegPressure.h
+++ b/llvm/lib/Target/AMDGPU/GCNRegPressure.h
@@ -428,10 +428,20 @@ class GCNDownwardRPTracker : public GCNRPTracker {
return Res;
}
- /// Reset tracker to the point before the \p MI
- /// filling \p LiveRegs upon this point using LIS.
- /// \p returns false if block is empty except debug values.
- bool reset(const MachineInstr &MI, const LiveRegSet *LiveRegs = nullptr);
+ /// Reset tracker to the point before the \p MI filling \p LiveRegs upon this
+ /// point using LIS. \p End must be between the MI and the end of its parent
+ /// block (inclusive). \p returns false if the range [MI, End) is empty except
+ /// debug values, in which case the current/maximum pressure are not changed.
+ bool reset(const MachineInstr &MI, MachineBasicBlock::const_iterator End,
+ const LiveRegSet *LiveRegs = nullptr);
+
+ /// Reset tracker to the point before the \p MI filling \p LiveRegs upon this
+ /// point using LIS. \p returns false if there are only debug values between
+ /// \p MI (inclusive) and end of its parent block, in which case the
+ /// current/maximum pressure are not changed.
+ bool reset(const MachineInstr &MI, const LiveRegSet *LiveRegs = nullptr) {
+ return reset(MI, MI.getParent()->end(), LiveRegs);
+ }
/// Move to the state right before the next MI or after the end of MBB.
/// \p returns false if reached end of the block.
@@ -462,10 +472,16 @@ class GCNDownwardRPTracker : public GCNRPTracker {
/// \p MI and use LIS for RP calculations.
bool advance(MachineInstr *MI = nullptr, bool UseInternalIterator = true);
- /// Advance instructions until before \p End.
+ /// Advance instructions until before \p End using internal iterators to
+ /// process instructions in program order. Returns whether iterators actually
+ /// had to advance to reach \p End.
bool advance(MachineBasicBlock::const_iterator End);
- /// Reset to \p Begin and advance to \p End.
+ /// Reset tracker to \p Begin (filling \p LiveRegs upon this point using LIS)
+ /// and advance to \p End, which must be between \p Begin and the end of its
+ /// parent block (inclusive). \p returns false if the range [Begin, End) is
+ /// empty except debug values, in which case the current/maximum pressure are
+ /// not changed.
bool advance(MachineBasicBlock::const_iterator Begin,
MachineBasicBlock::const_iterator End,
const LiveRegSet *LiveRegsCopy = nullptr);
diff --git a/llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp b/llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp
index 5d20d1e10a0da..7a4774336c5e7 100644
--- a/llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp
+++ b/llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp
@@ -1029,8 +1029,13 @@ GCNScheduleDAGMILive::getRealRegPressure(unsigned RegionIdx) const {
if (Regions[RegionIdx].first == Regions[RegionIdx].second)
return llvm::getRegPressure(MRI, LiveIns[RegionIdx]);
GCNDownwardRPTracker RPTracker(*LIS);
- RPTracker.advance(Regions[RegionIdx].first, Regions[RegionIdx].second,
- &LiveIns[RegionIdx]);
+ if (!RPTracker.advance(Regions[RegionIdx].first, Regions[RegionIdx].second,
+ &LiveIns[RegionIdx])) {
+ // Advance can produce false on a non-empty region if all MIs in the region
+ // are debug values; in such cases the maintained max pressure is invalid
+ // and the only source of pressure are the region's live-ins.
+ return llvm::getRegPressure(MRI, LiveIns[RegionIdx]);
+ }
return RPTracker.moveMaxPressure();
}
@@ -2025,8 +2030,7 @@ GCNSchedStage::getScheduleMetrics(const std::vector<SUnit> &InputSchedule) {
#ifndef NDEBUG
LLVM_DEBUG(
printScheduleModel(ReadyCyclesSorted);
- dbgs() << "\n\t"
- << "Metric: "
+ dbgs() << "\n\t" << "Metric: "
<< (SumBubbles
? (SumBubbles * ScheduleMetrics::ScaleFactor) / CurrCycle
: 1)
diff --git a/llvm/unittests/Target/AMDGPU/GCNRegPressureTest.cpp b/llvm/unittests/Target/AMDGPU/GCNRegPressureTest.cpp
index ad84f4df65288..d36742e9b6d85 100644
--- a/llvm/unittests/Target/AMDGPU/GCNRegPressureTest.cpp
+++ b/llvm/unittests/Target/AMDGPU/GCNRegPressureTest.cpp
@@ -82,25 +82,15 @@ body: |
// which would return false in this case.
//
// There aren't any non-debug instruction between the beginning of bb1 and
- // Dbg1 (exclusive). However, the call to reset takes the end of the MBB as
- // the limit, so it pushes the beginning of the block up to %2's def and
- // considers the reset successful.
- EXPECT_TRUE(RPTracker.reset(*MBB1.begin(), &MBB1LiveIns));
- EXPECT_TRUE(RPTrackerNoLiveIns.reset(*MBB1.begin(), nullptr));
- // advance then unnecessarily processes instructions in order until the end
- // of the block, even though it is already past Dbg1. It still returns false
- // because it is stopped by the end of block delimiter, not the end
- // iterator.
- EXPECT_FALSE(RPTracker.advance(Dbg1));
- EXPECT_FALSE(RPTrackerNoLiveIns.advance(Dbg1));
-
- // In that case, the maximum pressure is also the pressure induced by the
- // block's live-ins plus %2's def i.e., 3 VGPRs. This is confusing because
- // %2's def is outside the [Begin,End) range we passed to advance, and there
- // is no indication that a false return value should make the tracked
- // pressure invalid.
- EXPECT_EQ(RPTracker.moveMaxPressure().getVGPRNum(false), 3U);
- EXPECT_EQ(RPTrackerNoLiveIns.moveMaxPressure().getVGPRNum(false), 3U);
+ // Dbg1 (exclusive), the reset is therefore unsuccessful. The advance caller
+ // returns early on a failure to reset.
+ EXPECT_FALSE(RPTracker.reset(*MBB1.begin(), Dbg1, &MBB1LiveIns));
+ EXPECT_FALSE(RPTrackerNoLiveIns.reset(*MBB1.begin(), Dbg1, nullptr));
+
+ // In that case, the maximum pressure is unchanged from the beginning since
+ // reset was unsuccessful.
+ EXPECT_EQ(RPTracker.moveMaxPressure().getVGPRNum(false), 0U);
+ EXPECT_EQ(RPTrackerNoLiveIns.moveMaxPressure().getVGPRNum(false), 0U);
}
}
@@ -140,17 +130,12 @@ body: |
// which would return true in this case.
//
// There aren't any non-debug instruction in bb.2, the reset is therefore
- // unsuccessful. However the advance caller discards that return value and
- // proceeds to calling its override.
- EXPECT_FALSE(RPTracker.reset(*MBB1.begin(), &MBB1LiveIns));
- EXPECT_FALSE(RPTrackerNoLiveIns.reset(*MBB1.begin(), nullptr));
- // advance then produces true even though no advancement actually happened.
- EXPECT_TRUE(RPTracker.advance(MBB1.end()));
- EXPECT_TRUE(RPTrackerNoLiveIns.advance(MBB1.end()));
+ // unsuccessful. The advance caller returns early on a failure to reset.
+ EXPECT_FALSE(RPTracker.reset(*MBB1.begin(), MBB1.end(), &MBB1LiveIns));
+ EXPECT_FALSE(RPTrackerNoLiveIns.reset(*MBB1.begin(), MBB1.end(), nullptr));
// In that case, the maximum pressure is unchanged from the beginning since
- // reset was unsuccessful. This is confusing because the top-level advance
- // call produced true, yet the block's live-in pressure was not considered.
+ // reset was unsuccessful.
EXPECT_EQ(RPTracker.moveMaxPressure().getVGPRNum(false), 0U);
EXPECT_EQ(RPTrackerNoLiveIns.moveMaxPressure().getVGPRNum(false), 0U);
}
>From 895b992f68a2365ea88e5d3218bc802f8dae241f Mon Sep 17 00:00:00 2001
From: Lucas Ramirez <lucas.rami at proton.me>
Date: Fri, 8 May 2026 15:27:51 +0000
Subject: [PATCH 2/2] Address feedback
---
llvm/lib/Target/AMDGPU/GCNRegPressure.cpp | 2 +-
llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp | 3 ++-
llvm/unittests/Target/AMDGPU/GCNRegPressureTest.cpp | 2 ++
3 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp b/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp
index 9b8e63a39fab8..4716ce9cc5b92 100644
--- a/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp
+++ b/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp
@@ -622,7 +622,7 @@ void GCNUpwardRPTracker::recede(const MachineInstr &MI) {
bool GCNDownwardRPTracker::reset(const MachineInstr &MI,
MachineBasicBlock::const_iterator End,
- const LiveRegSet *LiveRegsCopy) {
+ const LiveRegSet *LiveRegsCopy) {
MRI = &MI.getMF()->getRegInfo();
LastTrackedMI = nullptr;
MBBEnd = MI.getParent()->end();
diff --git a/llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp b/llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp
index 7a4774336c5e7..95dab1cebe5e3 100644
--- a/llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp
+++ b/llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp
@@ -2030,7 +2030,8 @@ GCNSchedStage::getScheduleMetrics(const std::vector<SUnit> &InputSchedule) {
#ifndef NDEBUG
LLVM_DEBUG(
printScheduleModel(ReadyCyclesSorted);
- dbgs() << "\n\t" << "Metric: "
+ dbgs() << "\n\t"
+ << "Metric: "
<< (SumBubbles
? (SumBubbles * ScheduleMetrics::ScaleFactor) / CurrCycle
: 1)
diff --git a/llvm/unittests/Target/AMDGPU/GCNRegPressureTest.cpp b/llvm/unittests/Target/AMDGPU/GCNRegPressureTest.cpp
index d36742e9b6d85..2d96990723c4c 100644
--- a/llvm/unittests/Target/AMDGPU/GCNRegPressureTest.cpp
+++ b/llvm/unittests/Target/AMDGPU/GCNRegPressureTest.cpp
@@ -86,6 +86,8 @@ body: |
// returns early on a failure to reset.
EXPECT_FALSE(RPTracker.reset(*MBB1.begin(), Dbg1, &MBB1LiveIns));
EXPECT_FALSE(RPTrackerNoLiveIns.reset(*MBB1.begin(), Dbg1, nullptr));
+ EXPECT_FALSE(RPTracker.advance(Dbg1));
+ EXPECT_FALSE(RPTrackerNoLiveIns.advance(Dbg1));
// In that case, the maximum pressure is unchanged from the beginning since
// reset was unsuccessful.
More information about the llvm-branch-commits
mailing list