[llvm] [AMDGPU] Fix inconsistencies in RP tracking advance/reset behavior (PR #196595)
via llvm-commits
llvm-commits at lists.llvm.org
Fri May 8 10:52:36 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-backend-amdgpu
Author: Lucas Ramirez (lucas-rami)
<details>
<summary>Changes</summary>
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.
---
Full diff: https://github.com/llvm/llvm-project/pull/196595.diff
4 Files Affected:
- (modified) llvm/lib/Target/AMDGPU/GCNRegPressure.cpp (+5-3)
- (modified) llvm/lib/Target/AMDGPU/GCNRegPressure.h (+22-6)
- (modified) llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp (+7-2)
- (modified) llvm/unittests/Target/AMDGPU/GCNRegPressureTest.cpp (+12-25)
``````````diff
diff --git a/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp b/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp
index 683e658aa4fb4..4716ce9cc5b92 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,
+ 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..95dab1cebe5e3 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();
}
diff --git a/llvm/unittests/Target/AMDGPU/GCNRegPressureTest.cpp b/llvm/unittests/Target/AMDGPU/GCNRegPressureTest.cpp
index ad84f4df65288..2d96990723c4c 100644
--- a/llvm/unittests/Target/AMDGPU/GCNRegPressureTest.cpp
+++ b/llvm/unittests/Target/AMDGPU/GCNRegPressureTest.cpp
@@ -82,25 +82,17 @@ 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.
+ // 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));
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);
+ // 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 +132,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);
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/196595
More information about the llvm-commits
mailing list