[AMDGPU][NFC] Refine representation of register intervals in SIInsertWaitcnts.

[kosarev]

Summary

Currently, we represent register intervals as pairs of corresponding encoding values. The main idea behind this this patch is to replace such pairs with structures that represent the intervals in a more abstract way, thus conceptually separating the encoding details from the rest of the logic in the pass.

The new structures are still essentially pairs of integers representing the interval boundaries, but now make no references to encodings and have more suitably named fields. Using the chance, the type of endpoints was changed to unsigned to match the types of values they are compared against. The interval structures were also made iterable to simplify the code.

[foad]

Can you please say more than "Refine" in the description? What's the point of this? Why is it better? I don't want to have to read the whole patch to try to guess the answers for myself.

[kosarev]

Update the commit description.

[kosarev]

Sorry, that's unintentionally!

[foad]

Currently, we represent register intervals as pairs of corresponding encoding values. The main idea behind this this patch is to replace such pairs with structures that represent the intervals in a more abstract way, thus conceptually separating the encoding details from the rest of the logic in the pass.

The new structures are still essentially pairs of integers representing the interval boundaries, but now make no references to encodings and have more suitably named fields.

Personally I don't see much benefit to the "make no references to encodings" abstraction unless you are planning to change things, such that the interval endpoints are no longer register encoding values. Are you?

Using the chance, the type of endpoints was changed to unsigned to match the types of values they are compared against.

Sure, seems nice, and it looks like {0, 0} works naturally as a placeholder so there was no need for the old code to use {-1, -1}.

The interval structures were also made iterable to simplify the code.

That is nice.

[kosarev]

Personally I don't see much benefit to the "make no references to encodings" abstraction unless you are planning to change things, such that the interval endpoints are no longer register encoding values. Are you?

I don't know about much benefit, but think there is some. As the pass only really uses encodings as implementation detail of how register operands are translated to intervals, segregating everything related to them in a small function simplifies the conceptual picture to readers of this code. It also encourages using that new translation function instead of replicating code that retrieves and manipulates encodings directly, which as any other deduplication might be considered a good thing regardless of anticipated changes.

[kosarev]

Let's postpone this until the benefit is more obvious.

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

 template <> struct enum_iteration_traits<InstCounterType> {
   static constexpr bool is_iterable = true;
 };
 } // namespace llvm
 
 namespace {
 auto inst_counter_types() { return enum_seq(VM_CNT, NUM_INST_CNTS); }
 
-using RegInterval = std::pair<int, int>;
-
 struct HardwareLimits {
   unsigned VmcntMax;
   unsigned ExpcntMax;
   unsigned LgkmcntMax;
   unsigned VscntMax;
 };
 
-struct RegisterEncoding {
-  unsigned VGPR0;
-  unsigned VGPRL;
-  unsigned SGPR0;
-  unsigned SGPRL;
+// TODO: Can ADT's AddressRange be generalized to implement this?
+struct RegInterval {
+  unsigned Start = 0;
+  unsigned End = 0;
+
+  auto seq() const { return llvm::seq(Start, End); }
+  auto begin() const { return seq().begin(); }
+  auto end() const { return seq().end(); }
+};
+
+struct RegPoolIntervals {
+  RegInterval VGPR;
+  RegInterval SGPR;
 };
 
 enum WaitEventType {
   VMEM_ACCESS,          // vector-memory read & write
   VMEM_READ_ACCESS,     // vector-memory read
   VMEM_WRITE_ACCESS,    // vector-memory write that is not scratch
   SCRATCH_WRITE_ACCESS, // vector-memory write that may be scratch
   LDS_ACCESS,           // lds read & write
enum VmemType {
   // BUF instructions and MIMG instructions without a sampler.
   VMEM_NOSAMPLER,
   // MIMG instructions with a sampler.
   VMEM_SAMPLER,
   // BVH instructions
   VMEM_BVH
 };
 
+static unsigned getRegPoint(unsigned Reg, const GCNSubtarget &ST) {
+  // Use encoding indexes to form register intervals.
+  const SIRegisterInfo &TRI = ST.getInstrInfo()->getRegisterInfo();
+  return TRI.getEncodingValue(AMDGPU::getMCReg(Reg, ST));
+}
+
 static bool updateVMCntOnly(const MachineInstr &Inst) {
   return SIInstrInfo::isVMEM(Inst) || SIInstrInfo::isFLATGlobal(Inst) ||
          SIInstrInfo::isFLATScratch(Inst);
 }
 
 VmemType getVmemType(const MachineInstr &Inst) {
   assert(updateVMCntOnly(Inst));
   if (!SIInstrInfo::isMIMG(Inst))

 // We also maintain the latest score for every event type that can change the
 // waitcnt in order to know if there are multiple types of events within
 // the brackets. When multiple types of event happen in the bracket,
 // wait count may get decreased out of order, therefore we need to put in
 // "s_waitcnt 0" before use.
 class WaitcntBrackets {
 public:
   WaitcntBrackets(const GCNSubtarget *SubTarget, HardwareLimits Limits,
-                  RegisterEncoding Encoding)
-      : ST(SubTarget), Limits(Limits), Encoding(Encoding) {}
+                  RegPoolIntervals Intervals)
+      : ST(SubTarget), Limits(Limits), Intervals(Intervals) {}
 
   unsigned getWaitCountMax(InstCounterType T) const {
     switch (T) {
     case VM_CNT:
       return Limits.VmcntMax;
     case LGKM_CNT:
       return Limits.LgkmcntMax;
     case EXP_CNT:
private:
   }
 
   void setExpScore(const MachineInstr *MI, const SIInstrInfo *TII,
                    const SIRegisterInfo *TRI, const MachineRegisterInfo *MRI,
                    unsigned OpNo, unsigned Val);
 
   const GCNSubtarget *ST = nullptr;
   HardwareLimits Limits = {};
-  RegisterEncoding Encoding = {};
+  RegPoolIntervals Intervals;
   unsigned ScoreLBs[NUM_INST_CNTS] = {0};
   unsigned ScoreUBs[NUM_INST_CNTS] = {0};
   unsigned PendingEvents = 0;
   // Remember the last flat memory operation.
   unsigned LastFlat[NUM_INST_CNTS] = {0};
   // wait_cnt scores for every vgpr.
   // Keep track of the VgprUB and SgprUB to make merge at join efficient.
   int VgprUB = -1;

 
 RegInterval WaitcntBrackets::getRegInterval(const MachineInstr *MI,
                                             const SIInstrInfo *TII,
                                             const MachineRegisterInfo *MRI,
                                             const SIRegisterInfo *TRI,
                                             unsigned OpNo) const {
   const MachineOperand &Op = MI->getOperand(OpNo);
   if (!TRI->isInAllocatableClass(Op.getReg()))
-    return {-1, -1};
+    return {};
 
   // A use via a PW operand does not need a waitcnt.
   // A partial write is not a WAW.
   assert(!Op.getSubReg() || !Op.isUndef());
 
   RegInterval Result;
 
-  unsigned Reg = TRI->getEncodingValue(AMDGPU::getMCReg(Op.getReg(), *ST));
+  unsigned Reg = getRegPoint(Op.getReg(), *ST);
 
   if (TRI->isVectorRegister(*MRI, Op.getReg())) {
-    assert(Reg >= Encoding.VGPR0 && Reg <= Encoding.VGPRL);
-    Result.first = Reg - Encoding.VGPR0;
+    assert(Reg >= Intervals.VGPR.Start && Reg <= Intervals.VGPR.End);
+    Result.Start = Reg - Intervals.VGPR.Start;
     if (TRI->isAGPR(*MRI, Op.getReg()))
-      Result.first += AGPR_OFFSET;
-    assert(Result.first >= 0 && Result.first < SQ_MAX_PGM_VGPRS);
+      Result.Start += AGPR_OFFSET;
+    assert(Result.Start < SQ_MAX_PGM_VGPRS);
   } else if (TRI->isSGPRReg(*MRI, Op.getReg())) {
-    assert(Reg >= Encoding.SGPR0 && Reg < SQ_MAX_PGM_SGPRS);
-    Result.first = Reg - Encoding.SGPR0 + NUM_ALL_VGPRS;
-    assert(Result.first >= NUM_ALL_VGPRS &&
-           Result.first < SQ_MAX_PGM_SGPRS + NUM_ALL_VGPRS);
+    assert(Reg >= Intervals.SGPR.Start && Reg < SQ_MAX_PGM_SGPRS);
+    Result.Start = Reg - Intervals.SGPR.Start + NUM_ALL_VGPRS;
+    assert(Result.Start >= NUM_ALL_VGPRS &&
+           Result.Start < SQ_MAX_PGM_SGPRS + NUM_ALL_VGPRS);
   }
   // TODO: Handle TTMP
   // else if (TRI->isTTMP(*MRI, Reg.getReg())) ...
   else
-    return {-1, -1};
+    return {};
 
   const TargetRegisterClass *RC = TII->getOpRegClass(*MI, OpNo);
   unsigned Size = TRI->getRegSizeInBits(*RC);
-  Result.second = Result.first + ((Size + 16) / 32);
+  Result.End = Result.Start + divideCeil(Size, 32);
 
   return Result;
 }
 
 void WaitcntBrackets::setExpScore(const MachineInstr *MI,
                                   const SIInstrInfo *TII,
                                   const SIRegisterInfo *TRI,
                                   const MachineRegisterInfo *MRI, unsigned OpNo,
                                   unsigned Val) {
   RegInterval Interval = getRegInterval(MI, TII, MRI, TRI, OpNo);
   assert(TRI->isVectorRegister(*MRI, MI->getOperand(OpNo).getReg()));
-  for (int RegNo = Interval.first; RegNo < Interval.second; ++RegNo) {
+  for (unsigned RegNo : Interval)
     setRegScore(RegNo, EXP_CNT, Val);
-  }
-}
 
 // MUBUF and FLAT LDS DMA operations need a wait on vmcnt before LDS written
 // can be accessed. A load from LDS to VMEM does not need a wait.
 static bool mayWriteLDSThroughDMA(const MachineInstr &MI) {
   return SIInstrInfo::isVALU(MI) &&
          (SIInstrInfo::isMUBUF(MI) || SIInstrInfo::isFLAT(MI)) &&
          MI.getOpcode() != AMDGPU::BUFFER_STORE_LDS_DWORD;
 }
if (TII->isDS(Inst) && (Inst.mayStore() || Inst.mayLoad())) {
         // For export the destination registers are really temps that
         // can be used as the actual source after export patching, so
         // we need to treat them like sources and set the EXP_CNT
         // score.
         for (unsigned I = 0, E = Inst.getNumOperands(); I != E; ++I) {
           MachineOperand &DefMO = Inst.getOperand(I);
           if (DefMO.isReg() && DefMO.isDef() &&
               TRI->isVGPR(*MRI, DefMO.getReg())) {
-            setRegScore(
-                TRI->getEncodingValue(AMDGPU::getMCReg(DefMO.getReg(), *ST)),
-                EXP_CNT, CurrScore);
+            setRegScore(getRegPoint(DefMO.getReg(), *ST), EXP_CNT, CurrScore);
           }
         }
       }
       for (unsigned I = 0, E = Inst.getNumOperands(); I != E; ++I) {
         MachineOperand &MO = Inst.getOperand(I);
         if (MO.isReg() && !MO.isDef() &&
             TRI->isVectorRegister(*MRI, MO.getReg())) {
           setExpScore(&Inst, TII, TRI, MRI, I, CurrScore);
         }
       }
     }
 #if 0 // TODO: check if this is handled by MUBUF code above.
   } else if (Inst.getOpcode() == AMDGPU::BUFFER_STORE_DWORD ||
        Inst.getOpcode() == AMDGPU::BUFFER_STORE_DWORDX2 ||
        Inst.getOpcode() == AMDGPU::BUFFER_STORE_DWORDX4) {
     MachineOperand *MO = TII->getNamedOperand(Inst, AMDGPU::OpName::data);
     unsigned OpNo;//TODO: find the OpNo for this operand;
     RegInterval Interval = getRegInterval(&Inst, TII, MRI, TRI, OpNo);
-    for (int RegNo = Interval.first; RegNo < Interval.second;
-    ++RegNo) {
+    for (unsigned RegNo : Interval)
       setRegScore(RegNo + NUM_ALL_VGPRS, t, CurrScore);
-    }
 #endif
   } else {
     // Match the score to the destination registers.
     for (unsigned I = 0, E = Inst.getNumOperands(); I != E; ++I) {
       auto &Op = Inst.getOperand(I);
       if (!Op.isReg() || !Op.isDef())
         continue;
       RegInterval Interval = getRegInterval(&Inst, TII, MRI, TRI, I);
       if (T == VM_CNT) {
-        if (Interval.first >= NUM_ALL_VGPRS)
+        if (Interval.Start >= NUM_ALL_VGPRS)
           continue;
         if (updateVMCntOnly(Inst)) {
           VmemType V = getVmemType(Inst);
-          for (int RegNo = Interval.first; RegNo < Interval.second; ++RegNo)
+          for (unsigned RegNo : Interval)
             VgprVmemTypes[RegNo] |= 1 << V;
         }
       }
-      for (int RegNo = Interval.first; RegNo < Interval.second; ++RegNo) {
+      for (unsigned RegNo : Interval)
         setRegScore(RegNo, T, CurrScore);
-      }
-    }
     if (Inst.mayStore() && (TII->isDS(Inst) || mayWriteLDSThroughDMA(Inst))) {
       setRegScore(SQ_MAX_PGM_VGPRS + EXTRA_VGPR_LDS, T, CurrScore);
     }
   }
 }
 
 void WaitcntBrackets::print(raw_ostream &OS) {
   OS << '\n';
if (MI.isCall() && callWaitsOnFunctionEntry(MI)) {
 
       int CallAddrOpIdx =
           AMDGPU::getNamedOperandIdx(MI.getOpcode(), AMDGPU::OpName::src0);
 
       if (MI.getOperand(CallAddrOpIdx).isReg()) {
         RegInterval CallAddrOpInterval =
           ScoreBrackets.getRegInterval(&MI, TII, MRI, TRI, CallAddrOpIdx);
 
-        for (int RegNo = CallAddrOpInterval.first;
-             RegNo < CallAddrOpInterval.second; ++RegNo)
+        for (unsigned RegNo : CallAddrOpInterval)
           ScoreBrackets.determineWait(LGKM_CNT, RegNo, Wait);
 
         int RtnAddrOpIdx =
           AMDGPU::getNamedOperandIdx(MI.getOpcode(), AMDGPU::OpName::dst);
         if (RtnAddrOpIdx != -1) {
           RegInterval RtnAddrOpInterval =
             ScoreBrackets.getRegInterval(&MI, TII, MRI, TRI, RtnAddrOpIdx);
 
-          for (int RegNo = RtnAddrOpInterval.first;
-               RegNo < RtnAddrOpInterval.second; ++RegNo)
+          for (unsigned RegNo : RtnAddrOpInterval)
             ScoreBrackets.determineWait(LGKM_CNT, RegNo, Wait);
         }
       }
     } else {
       // FIXME: Should not be relying on memoperands.
       // Look at the source operands of every instruction to see if
       // any of them results from a previous memory operation that affects
       // its current usage. If so, an s_waitcnt instruction needs to be
if (MI.isCall() && callWaitsOnFunctionEntry(MI)) {
         // If the instruction does not read tied source, skip the operand.
         if (Op.isTied() && Op.isUse() && TII->doesNotReadTiedSource(MI))
           continue;
 
         RegInterval Interval =
             ScoreBrackets.getRegInterval(&MI, TII, MRI, TRI, I);
 
         const bool IsVGPR = TRI->isVectorRegister(*MRI, Op.getReg());
-        for (int RegNo = Interval.first; RegNo < Interval.second; ++RegNo) {
+        for (unsigned RegNo : Interval) {
           if (IsVGPR) {
             // RAW always needs an s_waitcnt. WAW needs an s_waitcnt unless the
             // previous write and this write are the same type of VMEM
             // instruction, in which case they're guaranteed to write their
             // results in order anyway.
             if (Op.isUse() || !updateVMCntOnly(MI) ||
                 ScoreBrackets.hasOtherPendingVmemTypes(RegNo,
                                                        getVmemType(MI))) {
for (MachineInstr &MI : *MBB) {
       }
       for (unsigned I = 0; I < MI.getNumOperands(); I++) {
         MachineOperand &Op = MI.getOperand(I);
         if (!Op.isReg() || !TRI->isVectorRegister(*MRI, Op.getReg()))
           continue;
         RegInterval Interval = Brackets.getRegInterval(&MI, TII, MRI, TRI, I);
         // Vgpr use
         if (Op.isUse()) {
-          for (int RegNo = Interval.first; RegNo < Interval.second; ++RegNo) {
+          for (unsigned RegNo : Interval) {
             // If we find a register that is loaded inside the loop, 1. and 2.
             // are invalidated and we can exit.
             if (VgprDef.contains(RegNo))
               return false;
             VgprUse.insert(RegNo);
             // If at least one of Op's registers is in the score brackets, the
             // value is likely loaded outside of the loop.
             if (Brackets.getRegScore(RegNo, VM_CNT) > Brackets.getScoreLB(VM_CNT)) {
               UsesVgprLoadedOutside = true;
               break;
             }
           }
         }
         // VMem load vgpr def
         else if (isVMEMOrFlatVMEM(MI) && MI.mayLoad() && Op.isDef())
-          for (int RegNo = Interval.first; RegNo < Interval.second; ++RegNo) {
+          for (unsigned RegNo : Interval) {
             // If we find a register that is loaded inside the loop, 1. and 2.
             // are invalidated and we can exit.
             if (VgprUse.contains(RegNo))
               return false;
             VgprDef.insert(RegNo);
           }
       }
     }
bool SIInsertWaitcnts::runOnMachineFunction(MachineFunction &MF) {
   Limits.LgkmcntMax = AMDGPU::getLgkmcntBitMask(IV);
   Limits.VscntMax = ST->hasVscnt() ? 63 : 0;
 
   unsigned NumVGPRsMax = ST->getAddressableNumVGPRs();
   unsigned NumSGPRsMax = ST->getAddressableNumSGPRs();
   assert(NumVGPRsMax <= SQ_MAX_PGM_VGPRS);
   assert(NumSGPRsMax <= SQ_MAX_PGM_SGPRS);
 
-  RegisterEncoding Encoding = {};
-  Encoding.VGPR0 = TRI->getEncodingValue(AMDGPU::VGPR0);
-  Encoding.VGPRL = Encoding.VGPR0 + NumVGPRsMax - 1;
-  Encoding.SGPR0 = TRI->getEncodingValue(AMDGPU::SGPR0);
-  Encoding.SGPRL = Encoding.SGPR0 + NumSGPRsMax - 1;
+  RegPoolIntervals Intervals = {};
+  Intervals.VGPR.Start = getRegPoint(AMDGPU::VGPR0, *ST);
+  Intervals.VGPR.End = Intervals.VGPR.Start + NumVGPRsMax - 1;
+  Intervals.SGPR.Start = getRegPoint(AMDGPU::SGPR0, *ST);
+  Intervals.SGPR.End = Intervals.SGPR.Start + NumSGPRsMax - 1;
 
   TrackedWaitcntSet.clear();
   BlockInfos.clear();
   bool Modified = false;
 
   if (!MFI->isEntryFunction()) {
     // Wait for any outstanding memory operations that the input registers may
     // depend on. We can't track them and it's better to do the wait after the
for (auto BII = BlockInfos.begin(), BIE = BlockInfos.end(); BII != BIE;
 
       if (BI.Incoming) {
         if (!Brackets)
           Brackets = std::make_unique<WaitcntBrackets>(*BI.Incoming);
         else
           *Brackets = *BI.Incoming;
       } else {
         if (!Brackets)
-          Brackets = std::make_unique<WaitcntBrackets>(ST, Limits, Encoding);
+          Brackets = std::make_unique<WaitcntBrackets>(ST, Limits, Intervals);
         else
-          *Brackets = WaitcntBrackets(ST, Limits, Encoding);
+          *Brackets = WaitcntBrackets(ST, Limits, Intervals);
       }
 
       Modified |= insertWaitcntInBlock(MF, *MBB, *Brackets);
       BI.Dirty = false;
 
       if (Brackets->hasPendingEvent()) {
         BlockInfo *MoveBracketsToSucc = nullptr;
         for (MachineBasicBlock *Succ : MBB->successors()) {