[AMDGPU] NFC: Provide RPTracker interface for external iterators

[jrbyrnes]

This is part of a series of PRs which enable using the GCNRPTrackers during scheduling. I've split them up to (hopefully) make reviewing easier. For context see #88797 .

This PR adds adds an interface to the existing GCNRPTrackers which provides a way to use the trackers with an externally managed iterator.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

Author: Jeffrey Byrnes (jrbyrnes)

Changes

This is part of a series of PRs which enable using the GCNRPTrackers during scheduling. I've split them up to (hopefully) make reviewing easier. For context see #88797 .

This PR adds adds an interface to the existing GCNRPTrackers which provides a way to use the trackers with an externally managed iterator.

---

Full diff: https://github.com/llvm/llvm-project/pull/93088.diff

2 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/GCNRegPressure.cpp (+46-24)
• (modified) llvm/lib/Target/AMDGPU/GCNRegPressure.h (+10-8)

diff --git a/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp b/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp
index 5c394e6d6296d..f1c4c8b397ddc 100644
--- a/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp
+++ b/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp
@@ -343,24 +343,25 @@ void GCNRPTracker::reset(const MachineInstr &MI,
   MaxPressure = CurPressure = getRegPressure(*MRI, LiveRegs);
 }
 
-////////////////////////////////////////////////////////////////////////////////
-// GCNUpwardRPTracker
-
-void GCNUpwardRPTracker::reset(const MachineRegisterInfo &MRI_,
-                               const LiveRegSet &LiveRegs_) {
+void GCNRPTracker::reset(const MachineRegisterInfo &MRI_,
+                         const LiveRegSet &LiveRegs_) {
   MRI = &MRI_;
   LiveRegs = LiveRegs_;
   LastTrackedMI = nullptr;
   MaxPressure = CurPressure = getRegPressure(MRI_, LiveRegs_);
 }
 
-void GCNUpwardRPTracker::recede(const MachineInstr &MI) {
+////////////////////////////////////////////////////////////////////////////////
+// GCNUpwardRPTracker
+
+bool GCNUpwardRPTracker::recede(const MachineInstr &MI, bool ShouldTrackIt) {
   assert(MRI && "call reset first");
 
-  LastTrackedMI = &MI;
+  if (ShouldTrackIt)
+    LastTrackedMI = &MI;
 
   if (MI.isDebugInstr())
-    return;
+    return false;
 
   // Kill all defs.
   GCNRegPressure DefPressure, ECDefPressure;
@@ -412,6 +413,7 @@ void GCNUpwardRPTracker::recede(const MachineInstr &MI) {
                           : max(CurPressure, MaxPressure);
 
   assert(CurPressure == getRegPressure(*MRI, LiveRegs));
+  return false;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -430,28 +432,44 @@ bool GCNDownwardRPTracker::reset(const MachineInstr &MI,
   return true;
 }
 
-bool GCNDownwardRPTracker::advanceBeforeNext() {
+bool GCNDownwardRPTracker::advanceBeforeNext(MachineInstr *MI,
+                                             bool ShouldTrackIt,
+                                             LiveIntervals *TheLIS) {
   assert(MRI && "call reset first");
-  if (!LastTrackedMI)
-    return NextMI == MBBEnd;
+  SlotIndex SI;
+  LiveIntervals *CurrLIS;
+  MachineInstr *CurrMI;
+  if (ShouldTrackIt) {
+    if (!LastTrackedMI)
+      return NextMI == MBBEnd;
+
+    assert(NextMI == MBBEnd || !NextMI->isDebugInstr());
+    CurrLIS = const_cast<LiveIntervals *>(&LIS);
+    CurrMI = const_cast<MachineInstr *>(LastTrackedMI);
+
+    SI = NextMI == MBBEnd
+             ? CurrLIS->getInstructionIndex(*LastTrackedMI).getDeadSlot()
+             : CurrLIS->getInstructionIndex(*NextMI).getBaseIndex();
+  }
 
-  assert(NextMI == MBBEnd || !NextMI->isDebugInstr());
+  else { //! ShouldTrackIt
+    CurrLIS = TheLIS;
+    SI = CurrLIS->getInstructionIndex(*MI).getBaseIndex();
+    CurrMI = MI;
+  }
 
-  SlotIndex SI = NextMI == MBBEnd
-                     ? LIS.getInstructionIndex(*LastTrackedMI).getDeadSlot()
-                     : LIS.getInstructionIndex(*NextMI).getBaseIndex();
   assert(SI.isValid());
 
   // Remove dead registers or mask bits.
   SmallSet<Register, 8> SeenRegs;
-  for (auto &MO : LastTrackedMI->operands()) {
+  for (auto &MO : CurrMI->operands()) {
     if (!MO.isReg() || !MO.getReg().isVirtual())
       continue;
     if (MO.isUse() && !MO.readsReg())
       continue;
     if (!SeenRegs.insert(MO.getReg()).second)
       continue;
-    const LiveInterval &LI = LIS.getInterval(MO.getReg());
+    const LiveInterval &LI = CurrLIS->getInterval(MO.getReg());
     if (LI.hasSubRanges()) {
       auto It = LiveRegs.end();
       for (const auto &S : LI.subranges()) {
@@ -481,15 +499,18 @@ bool GCNDownwardRPTracker::advanceBeforeNext() {
 
   LastTrackedMI = nullptr;
 
-  return NextMI == MBBEnd;
+  return ShouldTrackIt && (NextMI == MBBEnd);
 }
 
-void GCNDownwardRPTracker::advanceToNext() {
+void GCNDownwardRPTracker::advanceToNext(MachineInstr *MI, bool ShouldTrackIt) {
   LastTrackedMI = &*NextMI++;
   NextMI = skipDebugInstructionsForward(NextMI, MBBEnd);
 
+  MachineInstr *CurrMI =
+      ShouldTrackIt ? const_cast<MachineInstr *>(LastTrackedMI) : MI;
+
   // Add new registers or mask bits.
-  for (const auto &MO : LastTrackedMI->all_defs()) {
+  for (const auto &MO : CurrMI->all_defs()) {
     Register Reg = MO.getReg();
     if (!Reg.isVirtual())
       continue;
@@ -502,11 +523,12 @@ void GCNDownwardRPTracker::advanceToNext() {
   MaxPressure = max(MaxPressure, CurPressure);
 }
 
-bool GCNDownwardRPTracker::advance() {
-  if (NextMI == MBBEnd)
+bool GCNDownwardRPTracker::advance(MachineInstr *MI, bool ShouldTrackIt,
+                                   LiveIntervals *TheLIS) {
+  if (ShouldTrackIt && NextMI == MBBEnd)
     return false;
-  advanceBeforeNext();
-  advanceToNext();
+  advanceBeforeNext(MI, ShouldTrackIt, TheLIS);
+  advanceToNext(MI, ShouldTrackIt);
   return true;
 }
 
diff --git a/llvm/lib/Target/AMDGPU/GCNRegPressure.h b/llvm/lib/Target/AMDGPU/GCNRegPressure.h
index 752f53752fa68..8abbce138cf16 100644
--- a/llvm/lib/Target/AMDGPU/GCNRegPressure.h
+++ b/llvm/lib/Target/AMDGPU/GCNRegPressure.h
@@ -160,6 +160,9 @@ class GCNRPTracker {
              bool After);
 
 public:
+  // reset tracker and set live register set to the specified value.
+  void reset(const MachineRegisterInfo &MRI_, const LiveRegSet &LiveRegs_);
+
   // live regs for the current state
   const decltype(LiveRegs) &getLiveRegs() const { return LiveRegs; }
   const MachineInstr *getLastTrackedMI() const { return LastTrackedMI; }
@@ -180,12 +183,9 @@ class GCNUpwardRPTracker : public GCNRPTracker {
 public:
   GCNUpwardRPTracker(const LiveIntervals &LIS_) : GCNRPTracker(LIS_) {}
 
-  // reset tracker and set live register set to the specified value.
-  void reset(const MachineRegisterInfo &MRI_, const LiveRegSet &LiveRegs_);
-
   // reset tracker at the specified slot index.
   void reset(const MachineRegisterInfo &MRI, SlotIndex SI) {
-    reset(MRI, llvm::getLiveRegs(SI, LIS, MRI));
+    GCNRPTracker::reset(MRI, llvm::getLiveRegs(SI, LIS, MRI));
   }
 
   // reset tracker to the end of the MBB.
@@ -200,7 +200,7 @@ class GCNUpwardRPTracker : public GCNRPTracker {
   }
 
   // move to the state just before the MI (in program order).
-  void recede(const MachineInstr &MI);
+  bool recede(const MachineInstr &MI, bool ShouldTrackIt = true);
 
   // checks whether the tracker's state after receding MI corresponds
   // to reported by LIS.
@@ -242,13 +242,15 @@ class GCNDownwardRPTracker : public GCNRPTracker {
 
   // Move to the state right before the next MI or after the end of MBB.
   // Returns false if reached end of the block.
-  bool advanceBeforeNext();
+  bool advanceBeforeNext(MachineInstr *MI = nullptr, bool ShouldTrackIt = true,
+                         LiveIntervals *TheLIS = nullptr);
 
   // Move to the state at the MI, advanceBeforeNext has to be called first.
-  void advanceToNext();
+  void advanceToNext(MachineInstr *MI = nullptr, bool ShouldTrackIt = true);
 
   // Move to the state at the next MI. Returns false if reached end of block.
-  bool advance();
+  bool advance(MachineInstr *MI = nullptr, bool ShouldTrackIt = true,
+               LiveIntervals *TheLIS = nullptr);
 
   // Advance instructions until before End.
   bool advance(MachineBasicBlock::const_iterator End);

[vpykhtin]

I haven't found cases where advance or recede arecalled with ShouldTrackIt = false, and the semantics of the flag looks vague, can we avoid the flag?

[jrbyrnes]

I haven't found cases where advance or recede arecalled with ShouldTrackIt = false, and the semantics of the flag looks vague, can we avoid the flag?

Thanks for catching that, that was my mistake. We should be using the !ShouldTrackIt version during scheduling. I've corrected that in #93090 , and also corrected an issue with the way RP was calculated using the !ShouldTrackIt version.

I hope it is more clear in that latest version why ShouldTrackIt is needed. We cannot perform speculative RP queries (initCandidate) using the internal iterator, so we need a tracker which allows us to provide the NextMI. If we did not want to introduce a flag, we would need to implement a new tracker that doesn't use an internal iterator, as was done in #88797

[jrbyrnes]

In the latest, I've ported over RP speculation code from the generic trackers to GCN Trackers. While this does introduce code duplication, I thought this to be the better approach than to try to support subreg liveness in generic speculation and introduce generic tracker copies per speculation.

I've tried to organize the git history to ease the burden of review, please let me know if there's something I should do to assist further.

Commits named "Port *" are simple copy/paste of code./
Commits named "Fix * port issues" are minor changes to resolve API / build errors

The most interesting is "Modify behavior to support RP subreg aware RP speculation" which slightly modifies the behavior to support accurate RP speculation.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[arsenm]

This probably doesn't handle subregister defs that read the register correctly. I think you need to one of the other iterators, and MO.readsReg

[jrbyrnes]

Actually, I'm not quite sure what we're trying to accomplish here?

  // Definitions

  1.  undef %63.sub1:vreg_128 = COPY %49.sub18:areg_1024 
  2.  %49.sub1:areg_1024 = COPY %63.sub1:vreg_128
  3.  %63.sub0:vreg_128 = COPY %49.sub19:areg_1024

  // No uses of %49.sub19 or defs of %49.subx

If we were speculating the RP from schedule (1, 3, ..) we would invoke findUseBetween to find any uses of %49 between 3s original position and its candidate position. I think you are saying that we should consider the %49.sub1 def in 2. as an implicit use of %49.sub19? I know that the LIS requires subregister liveness to be modeled in this way somewhat (e.g. no multiple connected components), but from the perspective of RegisterPressure, It seems more accurate (and safe) to not model it this way -- that is, to only check use operands.

[arsenm]

It is a use, and I don't see how it could possibly be safer to ignore it as such. Deviation from the liveness model requires a much stronger rationale than this. I've fixed too many bugs from missing uses on subreg defs.

All of the rest of the code is following ordinary liveness tracking

[jrbyrnes]

Ultimately, RP should agree with RA interference. AFAICT, RA interference is determined by the subrange segments, rather than the main range with the strange liveness concerns -- I'm still analyzing.

In https://godbolt.org/z/YY4P7f9oe we have a use %49.sub0:areg_1024 occuring before a def of the same superreg %49.sub1:areg_1024. According to the subreg def is a use of other subregs, %49.sub0:areg_1024 should be live until the def %49.sub1:areg_1024. However, when assigning %2:agpr_32, RA sees there is no interference with %49.sub0:areg_1024 and assigns to the same PhysReg.

This is consistent with how the GCNTrackers currently calculate RP: we consider the use subrange and whether or not it is live at some given index

llvm-project/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp

Line 458 in 10407be

if (!S.liveAt(SI)) {

and don't check if there is some reaching subreg def.

[arsenm]

It's this instead of isUndef, not with

[arsenm]

In the latest, I've ported over RP speculation code from the generic trackers to GCN Trackers. While this does introduce code duplication, I thought this to be the better approach than to try to support subreg liveness in generic speculation and introduce generic tracker copies per speculation.

The generic tracker really just should support subregister liveness

[jrbyrnes]

In the latest, I've ported over RP speculation code from the generic trackers to GCN Trackers. While this does introduce code duplication, I thought this to be the better approach than to try to support subreg liveness in generic speculation and introduce generic tracker copies per speculation.

The generic tracker really just should support subregister liveness

That is most likely the next step of this work.

In order to enable the GCNTrackers by default, in order to save compile time, we would need to modify the scheduler to allow for a mode whcih enables RP tracking but disables the generic RPTracker.

I think it probably makes more sense to just improve the accuracy of the generic trackers as you suggest. The purpose of this PR then is to provide access to the improvements while the longer term solution is being worked out.

[arsenm]

No decrease of pressure after?

[jrbyrnes]

see https://github.com/llvm/llvm-project/pull/93090/files/80e066cd1da82d1d55b74d2b3cab62d8934d9947#r1774010200

[jrbyrnes]

Landed with 17bc959