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 <[email protected]> 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 <[email protected]> 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. _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
