[AMDGPU] hazard recognizer for fp atomic to s_denorm_mode

[rampitec]

Summary

This requires 3 wait states unless there is a wait or VALU in
between

[arsenm]

lib/Target/AMDGPU/GCNHazardRecognizer.cpp

148

Typo Denrom

1156–1157

Assuming false for memoperands_empty is wrong

1157–1163

This whole approach to identifying the atomic opcodes doesn't really work. You can't rely on any of the information in the memoperand, especially the IR pointee type. We do have an SIInstrFlags::maybeAtomic, but I think you need to check the opcode for if it's an FP one

1161–1163

Can't use the pointee type

test/CodeGen/AMDGPU/fp-atomic-to-s_denormmode.mir

39

Should test all the FP atomic opcodes

[rampitec]

lib/Target/AMDGPU/GCNHazardRecognizer.cpp

1157–1163

maybeAtomic will catch too much, including atomic loads and stores. I can use "AMDGPU::getAtomicRetOp(I->getOpcode()) != -1" to catch it.
However listing all possible opcodes is error prone and not future proof, especially given that many of them are just commented out currently. If you believe this value is unreliable I will have to use another bit in the MI flags. Does it sound good?

[arsenm]

lib/Target/AMDGPU/GCNHazardRecognizer.cpp

1157–1163

Another bit would be fine

[arsenm]

LGTM assuming the DS atomics really aren't impacted

lib/Target/AMDGPU/BUFInstructions.td

696 ↗

(On Diff #205910)

I didn't know you could omit the braces here

[rampitec]

In D63619#1553498, @arsenm wrote:

LGTM assuming the DS atomics really aren't impacted

According to the ticket that is only vmem.

lib/Target/AMDGPU/BUFInstructions.td

696 ↗

(On Diff #205910)

You do not need braces to cover a single def.

lib/Target/AMDGPU/GCNHazardRecognizer.h

private:
   int checkVALUHazards(MachineInstr *VALU);
   int checkVALUHazardsHelper(const MachineOperand &Def, const MachineRegisterInfo &MRI);
   int checkRWLaneHazards(MachineInstr *RWLane);
   int checkRFEHazards(MachineInstr *RFE);
   int checkInlineAsmHazards(MachineInstr *IA);
   int checkAnyInstHazards(MachineInstr *MI);
   int checkReadM0Hazards(MachineInstr *SMovRel);
   int checkNSAtoVMEMHazard(MachineInstr *MI);
+  int checkFPAtomicToDenormModeHazard(MachineInstr *MI);
   void fixHazards(MachineInstr *MI);
   bool fixVcmpxPermlaneHazards(MachineInstr *MI);
   bool fixVMEMtoScalarWriteHazards(MachineInstr *MI);
   bool fixSMEMtoVectorWriteHazards(MachineInstr *MI);
   bool fixVcmpxExecWARHazard(MachineInstr *MI);
   bool fixLdsBranchVmemWARHazard(MachineInstr *MI);
 
 public:

lib/Target/AMDGPU/GCNHazardRecognizer.cpp

GCNHazardRecognizer::getHazardType(SUnit *SU, int Stalls) {
   if ((SIInstrInfo::isVMEM(*MI) ||
        SIInstrInfo::isFLAT(*MI))
       && checkVMEMHazards(MI) > 0)
     return NoopHazard;
 
   if (ST.hasNSAtoVMEMBug() && checkNSAtoVMEMHazard(MI) > 0)
     return NoopHazard;
 
+  if (checkFPAtomicToDenormModeHazard(MI) > 0)
+    return NoopHazard;
+
   if (ST.hasNoDataDepHazard())
     return NoHazard;
 
   if (SIInstrInfo::isVALU(*MI) && checkVALUHazards(MI) > 0)
     return NoopHazard;
 
   if (SIInstrInfo::isDPP(*MI) && checkDPPHazards(MI) > 0)
     return NoopHazard;
if (SIInstrInfo::isSMRD(*MI))
     return std::max(WaitStates, checkSMRDHazards(MI));
 
   if (SIInstrInfo::isVMEM(*MI) || SIInstrInfo::isFLAT(*MI))
     WaitStates = std::max(WaitStates, checkVMEMHazards(MI));
 
   if (ST.hasNSAtoVMEMBug())
     WaitStates = std::max(WaitStates, checkNSAtoVMEMHazard(MI));
 
+  WaitStates = std::max(WaitStates, checkFPAtomicToDenormModeHazard(MI));
+
   if (ST.hasNoDataDepHazard())
     return WaitStates;
 
   if (SIInstrInfo::isVALU(*MI))
     WaitStates = std::max(WaitStates, checkVALUHazards(MI));
 
   if (SIInstrInfo::isDPP(*MI))
     WaitStates = std::max(WaitStates, checkDPPHazards(MI));
if (!SIInstrInfo::isMIMG(*I))
       return false;
     const AMDGPU::MIMGInfo *Info = AMDGPU::getMIMGInfo(I->getOpcode());
     return Info->MIMGEncoding == AMDGPU::MIMGEncGfx10NSA &&
            TII->getInstSizeInBytes(*I) >= 16;
   };
 
   return NSAtoVMEMWaitStates - getWaitStatesSince(IsHazardFn, 1);
 }
+
+int GCNHazardRecognizer::checkFPAtomicToDenormModeHazard(MachineInstr *MI) {
+  int FPAtomicToDenormModeWaitStates = 3;
+
+  if (MI->getOpcode() != AMDGPU::S_DENORM_MODE)
+    return 0;
+
+  auto IsHazardFn = [] (MachineInstr *I) {
+    if (!SIInstrInfo::isVMEM(*I) && !SIInstrInfo::isFLAT(*I))
+      return false;
+    if (!I->mayLoad() || !I->mayStore())
+      return false;
+    if (I->memoperands_empty())
+      return true; // Assume worst.
+    const MachineMemOperand *MemOp = *I->memoperands_begin();
+    if (!MemOp->isAtomic())
+      return false;
+    const Value *V = MemOp->getValue();
+    return !V || !V->getType()->isPointerTy() || // Assume worst.
+           V->getType()->getPointerElementType()->isFloatTy();
+  };
+
+  auto IsExpiredFn = [] (MachineInstr *MI, int WaitStates) {
+    if (WaitStates >= 3 || SIInstrInfo::isVALU(*MI))
+      return true;
+
+    switch (MI->getOpcode()) {
+    case AMDGPU::S_WAITCNT:
+    case AMDGPU::S_WAITCNT_VSCNT:
+    case AMDGPU::S_WAITCNT_VMCNT:
+    case AMDGPU::S_WAITCNT_EXPCNT:
+    case AMDGPU::S_WAITCNT_LGKMCNT:
+    case AMDGPU::S_WAITCNT_IDLE:
+      return true;
+    default:
+      break;
+    }
+
+    return false;
+  };
+
+
+  return FPAtomicToDenormModeWaitStates -
+         ::getWaitStatesSince(IsHazardFn, MI, IsExpiredFn);
+}

test/CodeGen/AMDGPU/fp-atomic-to-s_denormmode.mir

+# RUN: llc -march=amdgcn -mcpu=gfx1000 -verify-machineinstrs -run-pass post-RA-hazard-rec -o - %s | FileCheck -check-prefix=GCN %s
+
+# GCN-LABEL: name: flat_fp_atomic_to_s_denorm_mode
+# GCN:      FLAT_ATOMIC_FMIN
+# GCN-NEXT: S_NOP 0
+# GCN-NEXT: S_NOP 0
+# GCN-NEXT: S_NOP 0
+# GCN-NEXT: S_DENORM_MODE
+---
+name:            flat_fp_atomic_to_s_denorm_mode
+body:            |
+  bb.0:
+    FLAT_ATOMIC_FMIN undef %0:vreg_64, undef %1:vgpr_32, 0, 0, implicit $exec, implicit $flat_scr :: (volatile load store seq_cst seq_cst 4 on `float addrspace(1)* undef`)
+    S_DENORM_MODE 0
+...
+
+# GCN-LABEL: name: flat_fp_atomic_to_s_denorm_mode_waitcnt
+# GCN:      FLAT_ATOMIC_FMIN
+# GCN-NEXT: S_WAITCNT
+# GCN-NEXT: S_DENORM_MODE
+---
+name:            flat_fp_atomic_to_s_denorm_mode_waitcnt
+body:            |
+  bb.0:
+    FLAT_ATOMIC_FMIN undef %0:vreg_64, undef %1:vgpr_32, 0, 0, implicit $exec, implicit $flat_scr :: (volatile load store seq_cst seq_cst 4 on `float addrspace(1)* undef`)
+    S_WAITCNT 0
+    S_DENORM_MODE 0
+...
+
+# GCN-LABEL: name: flat_fp_atomic_to_s_denorm_mode_valu
+# GCN:      FLAT_ATOMIC_FMIN
+# GCN-NEXT: V_ADD_F32_e32
+# GCN-NEXT: S_DENORM_MODE
+---
+name:            flat_fp_atomic_to_s_denorm_mode_valu
+body:            |
+  bb.0:
+    FLAT_ATOMIC_FMIN undef %0:vreg_64, undef %1:vgpr_32, 0, 0, implicit $exec, implicit $flat_scr :: (volatile load store seq_cst seq_cst 4 on `float addrspace(1)* undef`)
+    %2:vgpr_32 = V_ADD_F32_e32 undef %1:vgpr_32, undef %1:vgpr_32, implicit $exec
+    S_DENORM_MODE 0
+...