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); }