AMDGPU/SI: Don't use non-0 waitcnt values when waiting on Flat instructions

[• tstellarAMD]

Summary

Flat instruction can return out of order, so we need always need to wait
for all the outstanding flat operations.

[arsenm]

lib/Target/AMDGPU/SIInsertWaits.cpp

300–301

Is this too strict? I know the manual says something like the only sensible value to use is 0 ,but from the reasoning before it it sounds like that's only if accessing a generic address. We could check the MMO and see if it is really global which is the common case

[• tstellarAMD]

Only treat flat operations as unordered if they access the flat address space.

[arsenm]

LGTM with test fixed

test/CodeGen/MIR/AMDGPU/waitcnt.mir

51

This looks like it has a mem operand although the comment on the check line says it doesn't

[• tstellarAMD]

test/CodeGen/MIR/AMDGPU/waitcnt.mir

51

The first load is the one without the mem operand, I can clarify this in the comment.

[tony-tye]

Feedback on overall pass.

lib/Target/AMDGPU/SIInsertWaits.cpp

54

Is there a named constant for the maximum register number rather than using 512?

88

Would be helpful to state what the bits mean. It seems 1 is EXPORT and 2 is MEM-WRITE and perhaps have an enumeration that is used.

Would need to add 4 for GDS when supported.

97

Given that this is only for flat instructions that can complete early, not all flat, should this be renamed?

137

instrucitons -> instructions

193

Are BUFFER_CACHE_INV* marked as updating vmcnt?
Are FLAT* marked as updating vmcnt?
Are GDS instructions marked as lgkmcnt and expcnt?
GDS needs waitcnt 0 before EXEC can be updated.

197

Only GFX6 uses exp_cnt for stores.

Later targets do not increment this count, but stores of more than 2 dwords have a hardware hazard that requires at lease one instruction between the store and the next write of the register.

So EXP_CNT property should only be put on M*BUF instructions for GFX6 and not later.

199

// LGKM counters may be incremented by more than 1.

200

Are S_DCACHE_* marked as updating lgkmcnt?
Are FLAT* marked as updating lgkmcnt?

202

This check should also apply to scalar writes.

216

The scalar data cache invalidate and writeback instructions do not affect the lgkm counter so should not be marked in the td file as affecting the counter.

224

Why is this needed as Result is initialized to 0?

241

also GDS

246

Are any source operands that are registers with a counter value that has not yet been satisfied (ie counter < value already waitedOn)?

268–275

Only GFX6 requires to use the expcnt to determine if the input value is InUse.
There is also a hardware hazard if input is larger than N dwords which requires M instructions before register can be used as a destination (is that hazard checked?).

300–301

If the flat operation is known not to access LDS then it cannot return out of order. For example, flat is used in 64 bit to access the global address space. So wonder if should also check the address space of the operation and only do this if the address space is FLAT (and not when GLOBAL)?

316

Should this be querying a subtarget feature instead of a specific target generation? The feature here seems to be that soft clauses are supported.

320–325

This comment indicates that both SMEM and VMEM clauses must be broken, but the following code only handles VMEM as SMEM is handled elsewhere.

The rules for VMEM only have to be followed when XNACK is supported. However, the rules for SMEM need to be followed regardless of whether XNACK is enabled as SMEM operations can complete out of order.

326–331

Don't VMEM clauses only have to ensure input registers are not modified inside the clause when XNACK is being supported? We now have a subtarget feature to indicate that so should that be used here instead of checking the generation? So should this NOP insertion only be done when the XNACK feature is enabled?

333

Should this also include scalar writes?

333–337

Why is this if nested inside the enclosing if? Seems tracking the lastOpcodeType should be done regardless of breaking the soft clauses for consistency.

341

Add another bit for GDS.
Exports are kept in order only within each export type (color/null, position, parameter cache) so need separate bits.

359

For UsedRegs only the expcnt needs to be waited on before the register is available. For VMEM store in GFX6 both vmcnt and expcnt will be present in Limit; for GDS both expcnt and lgkm willbe present in Limit. So should this just update the expcnt?

377–378

This is conservatively correct. But a better approach would be to not increment the vmcnt for flat instructions that are to generic address space, and record in the DefinedRegs of the destination as maxint. That would allow non-0 vmcnt for using registers produced by non flat instructions (it would be conservative as the value would assume the flat may have completed early), and 0 for the result of the flat instruction.

381

Is this still true with current hardware? Pre-SI I think this was the case, but I thought SI onwards no longer used the export counter for VMEM instructions?

382

Should this be != 3? If both are seen then it will be 3, so 3 means it is NOT ordered, not that it IS ordered?
If adding other bits for GDS and export types then better to use BitCount(ExpInstrTypesSeen ) == 1

385

Currently the LGKM counter is always assumed unordered but this could be improved by tracking the classes of instruction that update it (as is done for EXP_CNT) and then can use non-0 waitcnt when only a single class of instructions have been seen since the last waitcnt for LGKM. This would potentially benefit the DS_* instructions greatly.

401

If Required is 0 then no wait is needed on this counter so Value should be set to Hardware limit. Only if Required is non 0 does it mean that there is an instruction in this BB that we must wait on.

459

Need delayed waitcnt if Counts is trying to wait on an instruction after the WaitedOn. So this should be:

if (Counts.Array[i] < LastIssued.Array[i] - WaitedOn.Array[i])

487

Only the expcnt should be considered. When UseRegs is set it includes all counters and we do not need to wait for a GFX6 store to complete before being able to use the source register. That only has to be waited for before using the destination register.

501

Should this be a target feature?

558

Seem better if this was a target feature that was tested.

lib/Target/AMDGPU/SIInsertWaits.cpp

 } Counters;
 
 typedef enum {
   OTHER,
   SMEM,
   VMEM
 } InstType;
 
 typedef Counters RegCounters[512];
 typedef std::pair<unsigned, unsigned> RegInterval;
 
 class SIInsertWaits : public MachineFunctionPass {
 
 private:
   const SISubtarget *ST;
   const SIInstrInfo *TII;
   const SIRegisterInfo *TRI;
private:
   Counters LastIssued;
 
   /// \brief Registers used by async instructions.
   RegCounters UsedRegs;
 
   /// \brief Registers defined by async instructions.
   RegCounters DefinedRegs;
 
   /// \brief Different export instruction types seen since last wait.
   unsigned ExpInstrTypesSeen;
 
   /// \brief Type of the last opcode.
   InstType LastOpcodeType;
 
   bool LastInstWritesM0;
 
+  /// Whether or not we have flat operations outstanding.
+  bool IsFlatOutstanding;
+
   /// \brief Whether the machine function returns void
   bool ReturnsVoid;
 
   /// Whether the VCCZ bit is possibly corrupt
   bool VCCZCorrupt;
 
   /// \brief Get increment/decrement amount for this instruction.
   Counters getHwCounts(MachineInstr &MI);
private:
   bool unorderedDefines(MachineInstr &MI);
 
   /// \brief Resolve all operand dependencies to counter requirements
   Counters handleOperands(MachineInstr &MI);
 
   /// \brief Insert S_NOP between an instruction writing M0 and S_SENDMSG.
   void handleSendMsg(MachineBasicBlock &MBB, MachineBasicBlock::iterator I);
 
   /// Return true if there are LGKM instrucitons that haven't been waited on
   /// yet.
   bool hasOutstandingLGKM() const;
 
 public:
   static char ID;
 
   SIInsertWaits() :
     MachineFunctionPass(ID),

 bool SIInsertWaits::hasOutstandingLGKM() const {
   return WaitedOn.Named.LGKM != LastIssued.Named.LGKM;
 }
 
 Counters SIInsertWaits::getHwCounts(MachineInstr &MI) {
   uint64_t TSFlags = MI.getDesc().TSFlags;
   Counters Result = { { 0, 0, 0 } };
 
   Result.Named.VM = !!(TSFlags & SIInstrFlags::VM_CNT);
 
   // Only consider stores or EXP for EXP_CNT
   Result.Named.EXP = !!(TSFlags & SIInstrFlags::EXP_CNT &&
       (MI.getOpcode() == AMDGPU::EXP || MI.getDesc().mayStore()));
 
   // LGKM may uses larger values
   if (TSFlags & SIInstrFlags::LGKM_CNT) {
 
     if (TII->isSMRD(MI)) {
 
       if (MI.getNumOperands() != 0) {
         assert(MI.getOperand(0).isReg() &&
                "First LGKM operand must be a register!");
 
         // XXX - What if this is a write into a super register?
         const TargetRegisterClass *RC = TII->getOpRegClass(MI, 0);
         unsigned Size = RC->getSize();
         Result.Named.LGKM = Size > 4 ? 2 : 1;
       } else {
         // s_dcache_inv etc. do not have a a destination register. Assume we
         // want a wait on these.
         // XXX - What is the right value?
         Result.Named.LGKM = 1;
       }
     } else {
       // DS
       Result.Named.LGKM = 1;
     }
 
   } else {
     Result.Named.LGKM = 0;
   }
 
   return Result;
 }
 
 bool SIInsertWaits::isOpRelevant(MachineOperand &Op) {
   // Constants are always irrelevant
   if (!Op.isReg() || !TRI->isInAllocatableClass(Op.getReg()))
     return false;
 
   // Defines are always relevant
   if (Op.isDef())
     return true;
 
   // For exports all registers are relevant
   MachineInstr &MI = *Op.getParent();
   if (MI.getOpcode() == AMDGPU::EXP)
     return true;
 
   // For stores the stored value is also relevant
   if (!MI.getDesc().mayStore())
     return false;
 
   // Check if this operand is the value being stored.
   // Special case for DS/FLAT instructions, since the address
   // operand comes before the value operand and it may have
   // multiple data operands.
 
   if (TII->isDS(MI) || TII->isFLAT(MI)) {
     MachineOperand *Data = TII->getNamedOperand(MI, AMDGPU::OpName::data);
     if (Data && Op.isIdenticalTo(*Data))
       return true;
   }
 
   if (TII->isDS(MI)) {
     MachineOperand *Data0 = TII->getNamedOperand(MI, AMDGPU::OpName::data0);
     if (Data0 && Op.isIdenticalTo(*Data0))
       return true;
 
     MachineOperand *Data1 = TII->getNamedOperand(MI, AMDGPU::OpName::data1);
     return Data1 && Op.isIdenticalTo(*Data1);
   }
 
   // NOTE: This assumes that the value operand is before the
   // address operand, and that there is only one value operand.
   for (MachineInstr::mop_iterator I = MI.operands_begin(),
        E = MI.operands_end(); I != E; ++I) {
 
     if (I->isReg() && I->isUse())
       return Op.isIdenticalTo(*I);
   }
 
   return false;
 }
 
 RegInterval SIInsertWaits::getRegInterval(const TargetRegisterClass *RC,
                                           const MachineOperand &Reg) const {
   unsigned Size = RC->getSize();
   assert(Size >= 4);
 
   RegInterval Result;
   Result.first = TRI->getEncodingValue(Reg.getReg());
   Result.second = Result.first + Size / 4;
 
   return Result;
 }
 
 void SIInsertWaits::pushInstruction(MachineBasicBlock &MBB,
                                     MachineBasicBlock::iterator I,
                                     const Counters &Increment) {
 
   // Get the hardware counter increments and sum them up
   Counters Limit = ZeroCounts;
   unsigned Sum = 0;
 
+  if (TII->isFLAT(*I))
+    IsFlatOutstanding = true;
+
   for (unsigned i = 0; i < 3; ++i) {
     LastIssued.Array[i] += Increment.Array[i];
     if (Increment.Array[i])
       Limit.Array[i] = LastIssued.Array[i];
     Sum += Increment.Array[i];
   }
 
   // If we don't increase anything then that's it
   if (Sum == 0) {
     LastOpcodeType = OTHER;
     return;
   }
 
   if (ST->getGeneration() >= SISubtarget::VOLCANIC_ISLANDS) {
     // Any occurrence of consecutive VMEM or SMEM instructions forms a VMEM
     // or SMEM clause, respectively.
     //
     // The temporary workaround is to break the clauses with S_NOP.
     //
     // The proper solution would be to allocate registers such that all source
     // and destination registers don't overlap, e.g. this is illegal:
     //   r0 = load r2
     //   r2 = load r0
     if (LastOpcodeType == VMEM && Increment.Named.VM) {
       // Insert a NOP to break the clause.
       BuildMI(MBB, I, DebugLoc(), TII->get(AMDGPU::S_NOP))
           .addImm(0);
       LastInstWritesM0 = false;
     }
 
     if (TII->isSMRD(*I))
       LastOpcodeType = SMEM;
     else if (Increment.Named.VM)
       LastOpcodeType = VMEM;
   }
 
   // Remember which export instructions we have seen
   if (Increment.Named.EXP) {
     ExpInstrTypesSeen |= I->getOpcode() == AMDGPU::EXP ? 1 : 2;
   }
 
   for (unsigned i = 0, e = I->getNumOperands(); i != e; ++i) {
     MachineOperand &Op = I->getOperand(i);
     if (!isOpRelevant(Op))
       continue;
 
     const TargetRegisterClass *RC = TII->getOpRegClass(*I, i);
     RegInterval Interval = getRegInterval(RC, Op);
     for (unsigned j = Interval.first; j < Interval.second; ++j) {
 
       // Remember which registers we define
       if (Op.isDef())
         DefinedRegs[j] = Limit;
 
       // and which one we are using
       if (Op.isUse())
         UsedRegs[j] = Limit;
     }
   }
 }
 
 bool SIInsertWaits::insertWait(MachineBasicBlock &MBB,
                                MachineBasicBlock::iterator I,
                                const Counters &Required) {
 
   // End of program? No need to wait on anything
   // A function not returning void needs to wait, because other bytecode will
   // be appended after it and we don't know what it will be.
   if (I != MBB.end() && I->getOpcode() == AMDGPU::S_ENDPGM && ReturnsVoid)
     return false;
 
   // Figure out if the async instructions execute in order
   bool Ordered[3];
 
-  // VM_CNT is always ordered
-  Ordered[0] = true;
+  // VM_CNT is always ordered except when there are flat instructions, which
+  // can return out of order.
+  Ordered[0] = !IsFlatOutstanding;
 
   // EXP_CNT is unordered if we have both EXP & VM-writes
   Ordered[1] = ExpInstrTypesSeen == 3;
 
   // LGKM_CNT is handled as always unordered. TODO: Handle LDS and GDS
   Ordered[2] = false;
 
   // The values we are going to put into the S_WAITCNT instruction
   Counters Counts = HardwareLimits;
 
   // Do we really need to wait?
   bool NeedWait = false;
 
   for (unsigned i = 0; i < 3; ++i) {
 
     if (Required.Array[i] <= WaitedOn.Array[i])
       continue;
 
     NeedWait = true;
 
     if (Ordered[i]) {
       unsigned Value = LastIssued.Array[i] - Required.Array[i];
 
       // Adjust the value to the real hardware possibilities.
       Counts.Array[i] = std::min(Value, HardwareLimits.Array[i]);
 
     } else
       Counts.Array[i] = 0;
 
     // Remember on what we have waited on.
bool SIInsertWaits::insertWait(MachineBasicBlock &MBB,
   BuildMI(MBB, I, DebugLoc(), TII->get(AMDGPU::S_WAITCNT))
     .addImm(encodeWaitcnt(IV,
                           Counts.Named.VM,
                           Counts.Named.EXP,
                           Counts.Named.LGKM));
 
   LastOpcodeType = OTHER;
   LastInstWritesM0 = false;
+  IsFlatOutstanding = true;
   return true;
 }
 
 /// \brief helper function for handleOperands
 static void increaseCounters(Counters &Dst, const Counters &Src) {
 
   for (unsigned i = 0; i < 3; ++i)
     Dst.Array[i] = std::max(Dst.Array[i], Src.Array[i]);
void SIInsertWaits::handleExistingWait(MachineBasicBlock::iterator I) {
   unsigned Imm = I->getOperand(0).getImm();
   Counters Counts, WaitOn;
 
   Counts.Named.VM = decodeVmcnt(IV, Imm);
   Counts.Named.EXP = decodeExpcnt(IV, Imm);
   Counts.Named.LGKM = decodeLgkmcnt(IV, Imm);
 
   for (unsigned i = 0; i < 3; ++i) {
     if (Counts.Array[i] <= LastIssued.Array[i])
       WaitOn.Array[i] = LastIssued.Array[i] - Counts.Array[i];
     else
       WaitOn.Array[i] = 0;
   }
 
   increaseCounters(DelayedWaitOn, WaitOn);
 }
 
for (unsigned i = 0, e = MI.getNumOperands(); i != e; ++i) {
     if (!Op.isReg() || !TRI->isInAllocatableClass(Op.getReg()))
       continue;
 
     const TargetRegisterClass *RC = TII->getOpRegClass(MI, i);
     RegInterval Interval = getRegInterval(RC, Op);
     for (unsigned j = Interval.first; j < Interval.second; ++j) {
 
       if (Op.isDef()) {
         increaseCounters(Result, UsedRegs[j]);
         increaseCounters(Result, DefinedRegs[j]);
       }
 
       if (Op.isUse())
         increaseCounters(Result, DefinedRegs[j]);
     }
   }
 
   return Result;
 }
 
 void SIInsertWaits::handleSendMsg(MachineBasicBlock &MBB,
                                   MachineBasicBlock::iterator I) {
   if (ST->getGeneration() < SISubtarget::VOLCANIC_ISLANDS)
     return;
 
   // There must be "S_NOP 0" between an instruction writing M0 and S_SENDMSG.
   if (LastInstWritesM0 && I->getOpcode() == AMDGPU::S_SENDMSG) {
     BuildMI(MBB, I, DebugLoc(), TII->get(AMDGPU::S_NOP)).addImm(0);
     LastInstWritesM0 = false;
     return;
   }
bool SIInsertWaits::runOnMachineFunction(MachineFunction &MF) {
   HardwareLimits.Named.EXP = getExpcntBitMask(IV);
   HardwareLimits.Named.LGKM = getLgkmcntBitMask(IV);
 
   WaitedOn = ZeroCounts;
   DelayedWaitOn = ZeroCounts;
   LastIssued = ZeroCounts;
   LastOpcodeType = OTHER;
   LastInstWritesM0 = false;
+  IsFlatOutstanding = false;
   ReturnsVoid = MF.getInfo<SIMachineFunctionInfo>()->returnsVoid();
 
   memset(&UsedRegs, 0, sizeof(UsedRegs));
   memset(&DefinedRegs, 0, sizeof(DefinedRegs));
 
   SmallVector<MachineInstr *, 4> RemoveMI;
 
   for (MachineFunction::iterator BI = MF.begin(), BE = MF.end();
        BI != BE; ++BI) {
 
     MachineBasicBlock &MBB = *BI;
     for (MachineBasicBlock::iterator I = MBB.begin(), E = MBB.end();
          I != E; ++I) {
 
       if (ST->getGeneration() <= SISubtarget::SEA_ISLANDS) {
         // There is a hardware bug on CI/SI where SMRD instruction may corrupt
         // vccz bit, so when we detect that an instruction may read from a
         // corrupt vccz bit, we need to:
         // 1. Insert s_waitcnt lgkm(0) to wait for all outstanding SMRD operations to
         //    complete.
         // 2. Restore the correct value of vccz by writing the current value
         //    of vcc back to vcc.
 

test/CodeGen/MIR/AMDGPU/waitcnt.mir

+# RUN: llc -march=amdgcn -mcpu=fiji -run-pass si-insert-waits  %s -o - | FileCheck %s
+
+--- |
+  define void @flat_zero_waitcnt() { ret void }
+...
+---
+
+# CHECK-LABEL: name: flat_zero_waitcnt
+
+# CHECK-LABEL: bb.0:
+# CHECK: FLAT_LOAD_DWORD
+# CHECK: FLAT_LOAD_DWORDX4
+# s_waitcnt vmcnt(0) lgkmcnt(0)
+# CHECK: S_WAITCNT 112
+
+name: flat_zero_waitcnt
+
+body: |
+  bb.0:
+    %vgpr0 = FLAT_LOAD_DWORD %vgpr1_vgpr2, 0, 0, 0, implicit %exec, implicit %flat_scr
+    %vgpr3_vgpr4_vgpr5_vgpr6 = FLAT_LOAD_DWORDX4 %vgpr7_vgpr8, 0, 0, 0, implicit %exec, implicit %flat_scr
+    %vgpr0 = V_MOV_B32_e32 %vgpr1, implicit %exec
+    S_ENDPGM
+...