AMDGPU: Fix SMEM WAR hazard for gfx10 readlane

[kerbowa]

Summary

Hazard recognizer fails to see hazard with V_READLANE_B32_gfx10.

[rampitec]

It is OK as a w/a, but real opcode should not appear that early.
@arsenm What is the reason to use MC opcode in the SIFrameLowering::emitEpilogue()?

BuildMI(MBB, MBBI, DL, TII->getMCOpcodeFromPseudo(AMDGPU::V_READLANE_B32),
        FuncInfo->getFrameOffsetReg())

[arsenm]

In D69172#1714808, @rampitec wrote:

It is OK as a w/a, but real opcode should not appear that early.
@arsenm What is the reason to use MC opcode in the SIFrameLowering::emitEpilogue()?

BuildMI(MBB, MBBI, DL, TII->getMCOpcodeFromPseudo(AMDGPU::V_READLANE_B32),
        FuncInfo->getFrameOffsetReg())

There is a reason for it, but I don't remember what it is. I've tried to fix this multiple times in the past, and then rediscovered why. I think it had something to do with an operand constraint/encoding change in VI

[arsenm]

In D69172#1714838, @arsenm wrote:

In D69172#1714808, @rampitec wrote:

It is OK as a w/a, but real opcode should not appear that early.
@arsenm What is the reason to use MC opcode in the SIFrameLowering::emitEpilogue()?

BuildMI(MBB, MBBI, DL, TII->getMCOpcodeFromPseudo(AMDGPU::V_READLANE_B32),
        FuncInfo->getFrameOffsetReg())

There is a reason for it, but I don't remember what it is. I've tried to fix this multiple times in the past, and then rediscovered why. I think it had something to do with an operand constraint/encoding change in VI

The same is also done in spillSGPR/restoreSGPR, the PEI version was just reproducing the same

[rampitec]

LGTM as a w/a.

[kerbowa]

r375265

[foad]

Hi @kerbowa, why is this needed for gfx10? The public RDNA documentation says:

4.5. Manually Inserted Wait States (NOPs)
Inserting S_NOP is never required to achieve correct operation.

Is this not the full story?

[rampitec]

In D69172#1946305, @foad wrote:

Hi @kerbowa, why is this needed for gfx10? The public RDNA documentation says:

4.5. Manually Inserted Wait States (NOPs)
Inserting S_NOP is never required to achieve correct operation.

Is this not the full story?

It does not have data hazards, but then it has some bugs. This one is SMEMtoVectorWriteHazard.

llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp

if (!ST.hasSMEMtoVectorWriteHazard())
     return false;
 
   if (!SIInstrInfo::isVALU(*MI))
     return false;
 
   unsigned SDSTName;
   switch (MI->getOpcode()) {
   case AMDGPU::V_READLANE_B32:
+  case AMDGPU::V_READLANE_B32_gfx10:
   case AMDGPU::V_READFIRSTLANE_B32:
     SDSTName = AMDGPU::OpName::vdst;
     break;
   default:
     SDSTName = AMDGPU::OpName::sdst;
     break;
   }
 

llvm/test/CodeGen/AMDGPU/smem-war-hazard.mir

 body: |
   bb.0:
     liveins: $sgpr0, $sgpr1, $sgpr3, $vgpr0
     $sgpr2 = S_LOAD_DWORD_IMM $sgpr0_sgpr1, 0, 0, 0
     $sgpr0 = V_READLANE_B32 $vgpr0, $sgpr3
     S_ENDPGM 0
 ...
 
+# GCN-LABEL: name: hazard_smem_war_readlane_gfx10
+# GCN:      S_LOAD_DWORD_IMM
+# GCN:      $sgpr_null = S_MOV_B32 0
+# GCN-NEXT: V_READLANE_B32_gfx10
+---
+name: hazard_smem_war_readlane_gfx10
+body: |
+  bb.0:
+    liveins: $sgpr0, $sgpr1, $sgpr3, $vgpr0
+    $sgpr2 = S_LOAD_DWORD_IMM $sgpr0_sgpr1, 0, 0, 0
+    $sgpr0 = V_READLANE_B32_gfx10 $vgpr0, $sgpr3
+    S_ENDPGM 0
+...
+
 # GCN-LABEL: name: hazard_smem_war_readfirstlane
 # GCN:      S_LOAD_DWORD_IMM
 # GCN:      $sgpr_null = S_MOV_B32 0
 # GCN-NEXT: V_READFIRSTLANE_B32
 ---
 name: hazard_smem_war_readfirstlane
 body: |
   bb.0:
     liveins: $sgpr0, $sgpr1, $vgpr0
     $sgpr2 = S_LOAD_DWORD_IMM $sgpr0_sgpr1, 0, 0, 0
     $sgpr0 = V_READFIRSTLANE_B32 $vgpr0, implicit $exec
     S_ENDPGM 0
 ...