[AMDGPU] Fix vccz after v_readlane/v_readfirstlane to vcc_lo/hi

[foad]

Summary

Up to gfx9, writes to vcc_lo and vcc_hi by instructions like
v_readlane and v_readfirstlane do not update vccz to reflect the new
value of vcc. Fix it by reusing part of the existing vccz bug handling
code, which inserts an "s_mov_b64 vcc, vcc" instruction to restore vccz
just before an instruction that needs the correct value.

[arsenm]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1422

I thought we already had a vccz bug subtarget feature check? Either way I want to limit getGeneration checks and keep them in a subtarget check

1425

This won't catch the implicit def in inline asm for example

[foad]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1422

Yes we have the hasReadVCCZBug feature mentioned a few lines above, but as I understand it that is a different and very specific bug.

I'm happy to add a new feature for this if necessary but I'm not sure what to call it. I'm not even sure if it's a bug or a feature. The best explanation I've heard so far is that it's just an artifact of the way vccz is implemented in the hardware, and that it doesn't affect gfx10 because the hardware implementation changed to support wave32. So maybe it should be under a "hasWave32Support" feature?

1425

I specifically wanted to avoid treating this instruction (from the test case) as a write to vcc, despite its implicit-def:

$vcc_hi = V_READFIRSTLANE_B32 killed $vgpr0, implicit $exec, implicit-def $vcc

What kind of inline asm are you thinking of?

[arsenm]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1422

It doesn't need to be a full subtarget feature, but I do prefer to keep the getGeneration checks consolidated inside the subtarget. I'm not familiar with this problem and not sure what to call it either

1425

Any inline asm that touches vcc will appear as an implicit-def. You can't know without context that an implicit-def isn't really modifying the register. I'm guessing this one is to model the super-register def?

[foad]

Use a new named subtarget feature method.

[foad]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1425

I don't know exactly why the readfirstlane has an implicit def of vcc. I still think the most conservative fix is to only look for explicit defs of vcc_lo/vcc_hi, which is what my patch does.

[arsenm]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1425

Looking only at explicit defs is less conservative, as it will miss real defs. It will miss both inlineasm/call like operations, as well as the standard VCC defs as the VOPC vcc def appears in the implicit def list

llvm/test/CodeGen/AMDGPU/reload-vcc-vccz.mir

48 ↗

(On Diff #227296)

Should add a test with an implicit def, e..g. INLINEASM

[foad]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1425

It is crucial that we treat this instruction:

$vcc_hi = V_READFIRSTLANE_B32 killed $vgpr0, implicit $exec, implicit-def $vcc

as a def of vcc_hi (which corrupts vccz) and NOT as a def of vcc (which fixes vccz), despite the implicit-def, otherwise we will not be fixing the bug.

I suppose I could handle this with something like:

if (Inst.definesRegister(VCC_LO) || Inst.definesRegister(VCC_HI))
  VCCZCorrect = false;
else if (Inst.definesRegister(VCC))
  VCCZCorrect = true;

where the "else" ensures that V_READFIRSTLANE_B32 will fall under the first condition not the second. Would you prefer that?

(Or, stepping back a bit, is it wrong for V_READFIRSTLANE_B32 to have an implicit-def of vcc in the first place?)

[arsenm]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1425

That may or may not work; I would have to re-check the details of how definesRegister/modifiesRegister are implemented. You should add a few tests stressing the implicit defs to be sure

The implicit def of the super register is probably correct. What I assume happened here is that the implicit def of the super register is necessary to keep the verifier happy. If you look at SIInstrInfo::copyPhysReg for example, it adds a def of the super register on the first copy so that the later subregister defs don't think they are reading undef other lanes:

if (Idx == 0)
  Builder.addReg(DestReg, RegState::Define | RegState::Implicit);

[foad]

Check implicit as well as explicit defs.

Add test cases for inlineasm.

Rebase on D70708.

[arsenm]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1428

Can you just check modifiesRegister(VCC) instead of trying all the subregs?

[foad]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1428

I don't think that would let me distinguish this instruction (which writes to vcc_lo and corrupts vccz) from a "normal" write to vcc, would it?

$vcc_hi = V_READFIRSTLANE_B32 killed $vgpr0, implicit $exec, implicit-def $vcc

[foad]

Ping!

[arsenm]

llvm/test/CodeGen/AMDGPU/vccz-corrupt-bug-workaround.mir

142

CHECK-NEXT would be less fragile

159

ditto

[foad]

Use CHECK-NEXT instead of CHECK-NOT.

[arsenm]

LGTM with comment since I didn't know there was a second vccz bug

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1429

Can you add a comment explaining the difference between the SI bug and the gfx9 bug?

[merge_guards_bot]

Unit tests: pass. 62248 tests passed, 0 failed and 816 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[foad]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1429

I wasn't involved in the SI bug fix so I simply reinstated the original comment from D16725, which had got lost when this pass was reimplemented in D31161.

llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h

public:
   }
 
   /// Extra wait hazard is needed in some cases before
   /// s_cbranch_vccnz/s_cbranch_vccz.
   bool hasReadVCCZBug() const {
     return getGeneration() <= SEA_ISLANDS;
   }
 
+  /// Writes to VCC_LO/VCC_HI update the VCCZ flag.
+  bool partialVCCWritesUpdateVCCZ() const {
+    return getGeneration() >= GFX10;
+  }
+
   /// A read of an SGPR by SMRD instruction requires 4 wait states when the SGPR
   /// was written by a VALU instruction.
   bool hasSMRDReadVALUDefHazard() const {
     return getGeneration() == SOUTHERN_ISLANDS;
   }
 
   /// A read of an SGPR by a VMEM instruction requires 5 wait states when the
   /// SGPR was written by a VALU Instruction.

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

bool SIInsertWaitcnts::insertWaitcntInBlock(MachineFunction &MF,
                                             WaitcntBrackets &ScoreBrackets) {
   bool Modified = false;
 
   LLVM_DEBUG({
     dbgs() << "*** Block" << Block.getNumber() << " ***";
     ScoreBrackets.dump();
   });
 
+  // Assume VCCZ is correct at basic block boundaries, unless and until we need
+  // to handle cases where that is not true.
+  bool VCCZCorrect = true;
+
   // Walk over the instructions.
   MachineInstr *OldWaitcntInstr = nullptr;
 
   for (MachineBasicBlock::instr_iterator Iter = Block.instr_begin(),
                                          E = Block.instr_end();
        Iter != E;) {
     MachineInstr &Inst = *Iter;
 
     // Track pre-existing waitcnts from earlier iterations.
     if (Inst.getOpcode() == AMDGPU::S_WAITCNT ||
         (Inst.getOpcode() == AMDGPU::S_WAITCNT_VSCNT &&
          Inst.getOperand(0).isReg() &&
          Inst.getOperand(0).getReg() == AMDGPU::SGPR_NULL)) {
       if (!OldWaitcntInstr)
         OldWaitcntInstr = &Inst;
       ++Iter;
       continue;
     }
 
-    bool VCCZBugWorkAround = false;
+    bool RestoreVCCZ = false;
     if (readsVCCZ(Inst)) {
+      if (!VCCZCorrect)
+        RestoreVCCZ = true;
+      else if (ST->hasReadVCCZBug()) {
         if (ScoreBrackets.getScoreLB(LGKM_CNT) <
-              ScoreBrackets.getScoreUB(LGKM_CNT) &&
             ScoreBrackets.hasPendingEvent(SMEM_ACCESS)) {
-        if (ST->hasReadVCCZBug())
-          VCCZBugWorkAround = true;
+          RestoreVCCZ = true;
+        }
       }
     }
 
     if (TII->isSMRD(Inst)) {
       for (const MachineMemOperand *Memop : Inst.memoperands()) {
         const Value *Ptr = Memop->getValue();
         SLoadAddresses.insert(std::make_pair(Ptr, Inst.getParent()));
       }
     }
 
+    if (!ST->partialVCCWritesUpdateVCCZ()) {
+      // Up to gfx9, writes to vcc_lo and vcc_hi don't update vccz.
+      // Writes to vcc will fix it.
+      if (Inst.definesRegister(AMDGPU::VCC_LO) ||
+          Inst.definesRegister(AMDGPU::VCC_HI))
+        VCCZCorrect = false;
+      else if (Inst.definesRegister(AMDGPU::VCC))
+        VCCZCorrect = true;
+    }
+
     // Generate an s_waitcnt instruction to be placed before
     // cur_Inst, if needed.
     Modified |= generateWaitcntInstBefore(Inst, ScoreBrackets, OldWaitcntInstr);
     OldWaitcntInstr = nullptr;
 
     updateEventWaitcntAfter(Inst, &ScoreBrackets);
 
 #if 0 // TODO: implement resource type check controlled by options with ub = LB.
#endif
 
     LLVM_DEBUG({
       Inst.print(dbgs());
       ScoreBrackets.dump();
     });
 
     // TODO: Remove this work-around after fixing the scheduler and enable the
     // assert above.
-    if (VCCZBugWorkAround) {
+    if (RestoreVCCZ) {
       // Restore the vccz bit.  Any time a value is written to vcc, the vcc
       // bit is updated, so we can restore the bit by reading the value of
       // vcc and then writing it back to the register.
       BuildMI(Block, Inst, Inst.getDebugLoc(),
               TII->get(ST->isWave32() ? AMDGPU::S_MOV_B32 : AMDGPU::S_MOV_B64),
               TRI->getVCC())
           .addReg(TRI->getVCC());
+      VCCZCorrect = true;
       Modified = true;
     }
 
     ++Iter;
   }
 
   return Modified;
 }

llvm/test/CodeGen/AMDGPU/vccz-corrupt-bug-workaround.mir

bb.3:
     liveins: $vgpr0, $sgpr0_sgpr1_sgpr2_sgpr3:0x00000003
 
     $sgpr3 = S_MOV_B32 61440
     $sgpr2 = S_MOV_B32 -1
     BUFFER_STORE_DWORD_OFFSET killed $vgpr0, killed $sgpr0_sgpr1_sgpr2_sgpr3, 0, 0, 0, 0, 0, 0, 0, implicit $exec
     S_ENDPGM 0
 
 ...
+---
+# Test that after reloading vcc spilled to a vgpr, we insert any necessary
+# instructions to fix vccz.
+
+# CHECK-LABEL: name: reload_vcc_from_vgpr
+# CHECK: $vcc_lo = V_READLANE_B32_vi $vgpr0, 8, implicit-def $vcc
+# CHECK: $vcc_hi = V_READLANE_B32_vi $vgpr0, 9
+# SI:    $vcc = S_MOV_B64 $vcc
+# GFX9:  $vcc = S_MOV_B64 $vcc
+# CHECK-NEXT: S_CBRANCH_VCCNZ %bb.1, implicit killed $vcc
+
+name: reload_vcc_from_vgpr
+body: |
+  bb.0:
+    $vcc_lo = V_READLANE_B32_vi $vgpr0, 8, implicit-def $vcc
+    $vcc_hi = V_READLANE_B32_vi $vgpr0, 9
+    S_CBRANCH_VCCNZ %bb.1, implicit killed $vcc
+  bb.1:
+
+...
+---
+# Test that after reloading vcc spilled to memory, we insert any necessary
+# instructions to fix vccz.
+
+# CHECK-LABEL: name: reload_vcc_from_mem
+# CHECK: $vgpr0 = BUFFER_LOAD_DWORD_OFFSET $sgpr0_sgpr1_sgpr2_sgpr3, $sgpr4, 4, 0, 0, 0, 0, 0, implicit $exec
+# CHECK: $vcc_lo = V_READFIRSTLANE_B32 killed $vgpr0, implicit $exec, implicit-def $vcc
+# CHECK: $vgpr0 = BUFFER_LOAD_DWORD_OFFSET $sgpr0_sgpr1_sgpr2_sgpr3, $sgpr4, 8, 0, 0, 0, 0, 0, implicit $exec
+# CHECK: $vcc_hi = V_READFIRSTLANE_B32 killed $vgpr0, implicit $exec, implicit-def $vcc
+# SI:    $vcc = S_MOV_B64 $vcc
+# GFX9:  $vcc = S_MOV_B64 $vcc
+# CHECK-NEXT: S_CBRANCH_VCCNZ %bb.1, implicit killed $vcc
+
+name: reload_vcc_from_mem
+body: |
+  bb.0:
+    $vgpr0 = BUFFER_LOAD_DWORD_OFFSET $sgpr0_sgpr1_sgpr2_sgpr3, $sgpr4, 4, 0, 0, 0, 0, 0, implicit $exec
+    $vcc_lo = V_READFIRSTLANE_B32 killed $vgpr0, implicit $exec, implicit-def $vcc
+    $vgpr0 = BUFFER_LOAD_DWORD_OFFSET $sgpr0_sgpr1_sgpr2_sgpr3, $sgpr4, 8, 0, 0, 0, 0, 0, implicit $exec
+    $vcc_hi = V_READFIRSTLANE_B32 killed $vgpr0, implicit $exec, implicit-def $vcc
+    S_CBRANCH_VCCNZ %bb.1, implicit killed $vcc
+  bb.1:
+
+...
+---
+# Test that after inline asm that defines vcc_lo, we insert any necessary
+# instructions to fix vccz.
+
+# CHECK-LABEL: name: inlineasm_def_vcc_lo
+# CHECK: INLINEASM &"; def vcc_lo", 1, 10, implicit-def $vcc_lo
+# SI:    $vcc = S_MOV_B64 $vcc
+# GFX9:  $vcc = S_MOV_B64 $vcc
+# CHECK-NEXT: S_CBRANCH_VCCNZ %bb.1, implicit killed $vcc
+
+name: inlineasm_def_vcc_lo
+body: |
+  bb.0:
+    INLINEASM &"; def vcc_lo", 1, 10, implicit-def $vcc_lo
+    S_CBRANCH_VCCNZ %bb.1, implicit killed $vcc
+  bb.1:
+
+...
+---
+# Test that after inline asm that defines vcc, no unnecessary instructions are
+# inserted to fix vccz.
+
+# CHECK-LABEL: name: inlineasm_def_vcc
+# CHECK: INLINEASM &"; def vcc", 1, 10, implicit-def $vcc
+# CHECK-NEXT: S_CBRANCH_VCCNZ %bb.1, implicit killed $vcc
+
+name: inlineasm_def_vcc
+body: |
+  bb.0:
+    INLINEASM &"; def vcc", 1, 10, implicit-def $vcc
+    S_CBRANCH_VCCNZ %bb.1, implicit killed $vcc
+  bb.1:
+
+...