[AMDGPU] Ignore KILLs when forming clauses

[sebastian-ne]

Summary

KILL instructions are sometimes present and prevented hard
clauses from being formed.

Fix this by ignoring all meta instructions in clauses.

[foad]

llvm/lib/Target/AMDGPU/SIInsertHardClauses.cpp

80

What is SII for here?

129

I'm not sure if it's safe to use CI.Length instead of Size here. The difference is that if the clause has any trailing INTERNAL instructions then CI.Length includes them but Size does not. So with your changes, I would expect a sequence like this to be marked as a clause of length 4 instead of length 3:

load
internal
load
internal

[sebastian-ne]

Thanks for the review. You’re right about the too large clauses.
I fixed it and added it as a testcase.

llvm/lib/Target/AMDGPU/SIInsertHardClauses.cpp

80

Ups, I used that in an earlier version of this patch and forgot to remove it.

[foad]

I think this looks fine, thanks.

A possible alternative approach would be to put something like this at the start of emitClause:

while (getHardClauseType(CI.Last) == HARDCLAUSE_INTERNAL)
  --CI.Last;

llvm/lib/Target/AMDGPU/SIInsertHardClauses.cpp

61

Nit: maybe call them "meta instructions" here, for consistency with isMetaInstruction, and because "pseudo instruction" means something differrent in the AMDGPU backend.

120

"at the end"

188

Nit: do the == case first, otherwise the "else" case reads like a double negative.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

300

I don't see a reason to emit a comment for these. Kills for example already have a comment for them

[sebastian-ne]

Fix review comments

llvm/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

300

The problem is that the AsmPrinter only handles top-level instructions, but not instructions inside bundles.
So, without the code here, printing a bundle containing a KILL will crash because KILL and others are unhandled in AMDGPUInstPrinter::printInstruction.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

300

We shouldn't be bundling these?

[sebastian-ne]

llvm/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

300

How do we create the clause then?
In the tests, we have

$sgpr2 = S_LOAD_DWORD_IMM $sgpr0_sgpr1, 0, 0
KILL undef renamable $sgpr4
$sgpr3 = S_LOAD_DWORD_IMM $sgpr0_sgpr1, 4, 0

and we should be able to emit an s_clause 0x1 in front of the two memory instructions.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

300

Where did this kill come from? A kill of an undef source is pointless and can be deleted. The point of kill is to artifically extend live ranges anyway, so it can be moved outside of the bundle. Kills are ordinarily deleted after RA anyway

[sebastian-ne]

llvm/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

300

The case where I encountered it looks like this:

%470:sgpr_128 = S_LOAD_DWORDX4_IMM %469:sreg_64, 48, 0 :: (load (s128) from %ir.73, addrspace 4)
%25:sgpr_128 = S_LOAD_DWORDX4_IMM %3:sreg_64, 320, 0 :: (load (s128) from %ir.67, addrspace 4)
KILL %469.sub0:sreg_64, %469.sub1:sreg_64
%26:sgpr_128 = S_LOAD_DWORDX4_IMM %3:sreg_64, 480, 0 :: (load (s128) from %ir.70, addrspace 4)

The kill is inserted by the SIFormMemoryClauses pass.
In the above example, if flat scratch is enabled, the kill gets inserted after the third s_load, so a clause is inserted successfully later. Without flat scratch, no clause is formed.

[sebastian-ne]

llvm/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

300

Maybe SIFormMemoryClauses does not extend the potential clause to the last s_load because the register pressure estimate would get too high?
Therefore it inserts the KILL after the second s_load, which in turn prevents inserting a hard clause containing the latter two loads.

The would explain why the position of the KILL changes when flat scratch is enabled or disabled (enabling it frees the 4 SGPRs of the scratch SRD, so it has lower register pressure).

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

300

SIFormMemoryClauses's pressure estimate is extremely crude, so I wouldn't put so much faith in it (I think the pass needs a replacement overall, I think the allocator really needs to be aware of the restriction). The kill is there as a hint to the allocator, but doesn't need to stick around afterwards

[sebastian-ne]

llvm/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

300

Ok, should we remove the kill instructions if we encounter them in SIInsertHardClauses?
But that doesn’t help for other meta instructions like DBG instructions.

[sebastian-ne]

llvm/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

300

I guess this code is fine then, as we shouldn’t handle bundles in llvm’s generic code, so we need to handle everything in the targets.

llvm/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

if (MI->isBundle()) {
     }
 
     if (MI->getOpcode() == AMDGPU::SI_MASKED_UNREACHABLE) {
       if (isVerbose())
         OutStreamer->emitRawComment(" divergent unreachable");
       return;
     }
 
+    if (MI->isMetaInstruction()) {
+      if (isVerbose())
+        OutStreamer->emitRawComment(" meta instruction");
+      return;
+    }
+
     MCInst TmpInst;
     MCInstLowering.lower(MI, TmpInst);
     EmitToStreamer(*OutStreamer, TmpInst);
 
 #ifdef EXPENSIVE_CHECKS
     // Sanity-check getInstSizeInBytes on explicitly specified CPUs (it cannot
     // work correctly for the generic CPU).
     //

llvm/lib/Target/AMDGPU/SIInsertHardClauses.cpp

enum HardClauseType {
   HARDCLAUSE_SMEM,
   // VALU instructions.
   HARDCLAUSE_VALU,
   LAST_REAL_HARDCLAUSE_TYPE = HARDCLAUSE_VALU,
 
   // Internal instructions, which are allowed in the middle of a hard clause,
   // except for s_waitcnt.
   HARDCLAUSE_INTERNAL,
+  // Pseudo instructions that do not result in any ISA like KILL.
+  HARDCLAUSE_IGNORE,
   // Instructions that are not allowed in a hard clause: SALU, export, branch,
   // message, GDS, s_waitcnt and anything else not mentioned above.
   HARDCLAUSE_ILLEGAL,
 };
 
 class SIInsertHardClauses : public MachineFunctionPass {
 public:
   static char ID;
   const GCNSubtarget *ST = nullptr;
 
   SIInsertHardClauses() : MachineFunctionPass(ID) {}
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesCFG();
     MachineFunctionPass::getAnalysisUsage(AU);
   }
 
-  HardClauseType getHardClauseType(const MachineInstr &MI) {
+  HardClauseType getHardClauseType(const SIInstrInfo *SII,
+                                   const MachineInstr &MI) {
 
     // On current architectures we only get a benefit from clausing loads.
     if (MI.mayLoad()) {
       if (SIInstrInfo::isVMEM(MI) || SIInstrInfo::isSegmentSpecificFLAT(MI)) {
         if (ST->hasNSAClauseBug()) {
           const AMDGPU::MIMGInfo *Info = AMDGPU::getMIMGInfo(MI.getOpcode());
           if (Info && Info->MIMGEncoding == AMDGPU::MIMGEncGfx10NSA)
             return HARDCLAUSE_ILLEGAL;
         }
         return HARDCLAUSE_VMEM;
       }
       if (SIInstrInfo::isFLAT(MI))
         return HARDCLAUSE_FLAT;
       // TODO: LDS
       if (SIInstrInfo::isSMRD(MI))
         return HARDCLAUSE_SMEM;
     }
 
     // Don't form VALU clauses. It's not clear what benefit they give, if any.
 
     // In practice s_nop is the only internal instruction we're likely to see.
     // It's safe to treat the rest as illegal.
     if (MI.getOpcode() == AMDGPU::S_NOP)
       return HARDCLAUSE_INTERNAL;
+    if (MI.isMetaInstruction())
+      return HARDCLAUSE_IGNORE;
     return HARDCLAUSE_ILLEGAL;
   }
 
   // Track information about a clause as we discover it.
   struct ClauseInfo {
     // The type of all (non-internal) instructions in the clause.
     HardClauseType Type = HARDCLAUSE_ILLEGAL;
     // The first (necessarily non-internal) instruction in the clause.
     MachineInstr *First = nullptr;
     // The last non-internal instruction in the clause.
     MachineInstr *Last = nullptr;
     // The length of the clause including any internal instructions in the
     // middle or after the end of the clause.
     unsigned Length = 0;
     // The base operands of *Last.
     SmallVector<const MachineOperand *, 4> BaseOps;
   };
 
   bool emitClause(const ClauseInfo &CI, const SIInstrInfo *SII) {
-    // Get the size of the clause excluding any internal instructions at the
-    // end.
-    unsigned Size =
-        std::distance(CI.First->getIterator(), CI.Last->getIterator()) + 1;
-    if (Size < 2)
+    if (CI.First == CI.Last)
       return false;
-    assert(Size <= 64 && "Hard clause is too long!");
+    assert(CI.Length <= 64 && "Hard clause is too long!");
 
     auto &MBB = *CI.First->getParent();
     auto ClauseMI =
         BuildMI(MBB, *CI.First, DebugLoc(), SII->get(AMDGPU::S_CLAUSE))
-            .addImm(Size - 1);
+            .addImm(CI.Length - 1);
     finalizeBundle(MBB, ClauseMI->getIterator(),
                    std::next(CI.Last->getIterator()));
     return true;
   }
 
   bool runOnMachineFunction(MachineFunction &MF) override {
     if (skipFunction(MF.getFunction()))
       return false;
 
     ST = &MF.getSubtarget<GCNSubtarget>();
     if (!ST->hasHardClauses())
       return false;
 
     const SIInstrInfo *SII = ST->getInstrInfo();
     const TargetRegisterInfo *TRI = ST->getRegisterInfo();
 
     bool Changed = false;
     for (auto &MBB : MF) {
       ClauseInfo CI;
       for (auto &MI : MBB) {
-        HardClauseType Type = getHardClauseType(MI);
+        HardClauseType Type = getHardClauseType(SII, MI);
 
         int64_t Dummy1;
         bool Dummy2;
         unsigned Dummy3;
         SmallVector<const MachineOperand *, 4> BaseOps;
         if (Type <= LAST_REAL_HARDCLAUSE_TYPE) {
           if (!SII->getMemOperandsWithOffsetWidth(MI, BaseOps, Dummy1, Dummy2,
                                                   Dummy3, TRI)) {
             // We failed to get the base operands, so we'll never clause this
             // instruction with any other, so pretend it's illegal.
             Type = HARDCLAUSE_ILLEGAL;
           }
         }
 
         if (CI.Length == 64 ||
             (CI.Length && Type != HARDCLAUSE_INTERNAL &&
+             Type != HARDCLAUSE_IGNORE &&
              (Type != CI.Type ||
               // Note that we lie to shouldClusterMemOps about the size of the
               // cluster. When shouldClusterMemOps is called from the machine
               // scheduler it limits the size of the cluster to avoid increasing
               // register pressure too much, but this pass runs after register
               // allocation so there is no need for that kind of limit.
               !SII->shouldClusterMemOps(CI.BaseOps, BaseOps, 2, 2)))) {
           // Finish the current clause.
           Changed |= emitClause(CI, SII);
           CI = ClauseInfo();
         }
 
         if (CI.Length) {
           // Extend the current clause.
+          if (Type != HARDCLAUSE_IGNORE) {
             ++CI.Length;
             if (Type != HARDCLAUSE_INTERNAL) {
               CI.Last = &MI;
               CI.BaseOps = std::move(BaseOps);
             }
+          }
         } else if (Type <= LAST_REAL_HARDCLAUSE_TYPE) {
           // Start a new clause.
           CI = ClauseInfo{Type, &MI, &MI, 1, std::move(BaseOps)};
         }
       }
 
       // Finish the last clause in the basic block if any.
       if (CI.Length)

llvm/test/CodeGen/AMDGPU/gfx-callable-argument-types.ll

 ; GFX10-SCRATCH-NEXT:    v_writelane_b32 v40, s45, 9
 ; GFX10-SCRATCH-NEXT:    v_writelane_b32 v40, s46, 10
 ; GFX10-SCRATCH-NEXT:    v_writelane_b32 v40, s47, 11
 ; GFX10-SCRATCH-NEXT:    v_writelane_b32 v40, s48, 12
 ; GFX10-SCRATCH-NEXT:    v_writelane_b32 v40, s49, 13
 ; GFX10-SCRATCH-NEXT:    v_writelane_b32 v40, s50, 14
 ; GFX10-SCRATCH-NEXT:    v_writelane_b32 v40, s51, 15
 ; GFX10-SCRATCH-NEXT:    s_waitcnt lgkmcnt(0)
+; GFX10-SCRATCH-NEXT:    s_clause 0x2
 ; GFX10-SCRATCH-NEXT:    s_load_dword s2, s[0:1], 0x0
-; GFX10-SCRATCH-NEXT:    ; kill: killed $sgpr0_sgpr1
-; GFX10-SCRATCH-NEXT:    ; kill: killed $sgpr0_sgpr1
-; GFX10-SCRATCH-NEXT:    s_clause 0x1
+; GFX10-SCRATCH-NEXT:    ; meta instruction
+; GFX10-SCRATCH-NEXT:    ; meta instruction
 ; GFX10-SCRATCH-NEXT:    s_load_dwordx16 s[36:51], s[0:1], 0x40
 ; GFX10-SCRATCH-NEXT:    s_load_dwordx16 s[4:19], s[0:1], 0x0
 ; GFX10-SCRATCH-NEXT:    s_getpc_b64 s[0:1]
 ; GFX10-SCRATCH-NEXT:    s_add_u32 s0, s0, external_void_func_v32i32_i32_inreg@rel32@lo+4
 ; GFX10-SCRATCH-NEXT:    s_addc_u32 s1, s1, external_void_func_v32i32_i32_inreg@rel32@hi+12
 ; GFX10-SCRATCH-NEXT:    v_writelane_b32 v40, s30, 16
 ; GFX10-SCRATCH-NEXT:    v_writelane_b32 v40, s31, 17
 ; GFX10-SCRATCH-NEXT:    s_waitcnt lgkmcnt(0)

llvm/test/CodeGen/AMDGPU/hard-clauses.mir

bb.0:
     ; CHECK: liveins: $sgpr0_sgpr1_sgpr2_sgpr3_sgpr4_sgpr5_sgpr6_sgpr7, $sgpr8_sgpr9_sgpr10_sgpr11, $vgpr0, $vgpr1, $vgpr2, $vgpr3, $vgpr4, $vgpr5, $vgpr6, $vgpr7, $vgpr8
     ; CHECK: $vgpr10_vgpr11_vgpr12_vgpr13 = IMAGE_SAMPLE_D_V4_V9_nsa_gfx10 $vgpr3, $vgpr8, $vgpr7, $vgpr5, $vgpr4, $vgpr6, $vgpr0, $vgpr2, $vgpr2, $sgpr0_sgpr1_sgpr2_sgpr3_sgpr4_sgpr5_sgpr6_sgpr7, $sgpr8_sgpr9_sgpr10_sgpr11, 15, 2, 0, 0, 0, 0, 0, 0, 0, implicit $exec :: (load (s128))
     ; CHECK: $vgpr14 = IMAGE_SAMPLE_LZ_V1_V2_gfx10 $vgpr5_vgpr6, $sgpr0_sgpr1_sgpr2_sgpr3_sgpr4_sgpr5_sgpr6_sgpr7, $sgpr8_sgpr9_sgpr10_sgpr11, 1, 1, 0, 0, 0, 0, 0, 0, 0, implicit $exec :: (dereferenceable load (s128) from custom "ImageResource")
     ; CHECK: $vgpr20_vgpr21_vgpr22_vgpr23 = IMAGE_SAMPLE_D_V4_V9_nsa_gfx10 $vgpr3, $vgpr8, $vgpr7, $vgpr5, $vgpr4, $vgpr6, $vgpr0, $vgpr2, $vgpr2, $sgpr0_sgpr1_sgpr2_sgpr3_sgpr4_sgpr5_sgpr6_sgpr7, $sgpr8_sgpr9_sgpr10_sgpr11, 15, 2, 0, 0, 0, 0, 0, 0, 0, implicit $exec :: (load (s128))
     $vgpr10_vgpr11_vgpr12_vgpr13 = IMAGE_SAMPLE_D_V4_V9_nsa_gfx10 $vgpr3, $vgpr8, $vgpr7, $vgpr5, $vgpr4, $vgpr6, $vgpr0, $vgpr2, $vgpr2, $sgpr0_sgpr1_sgpr2_sgpr3_sgpr4_sgpr5_sgpr6_sgpr7, $sgpr8_sgpr9_sgpr10_sgpr11, 15, 2, 0, 0, 0, 0, 0, 0, 0, implicit $exec  :: (load (s128))
     $vgpr14 = IMAGE_SAMPLE_LZ_V1_V2_gfx10 $vgpr5_vgpr6, $sgpr0_sgpr1_sgpr2_sgpr3_sgpr4_sgpr5_sgpr6_sgpr7, $sgpr8_sgpr9_sgpr10_sgpr11, 1, 1, 0, 0, 0, 0, 0, 0, 0, implicit $exec :: (dereferenceable load (s128) from custom "ImageResource")
     $vgpr20_vgpr21_vgpr22_vgpr23 = IMAGE_SAMPLE_D_V4_V9_nsa_gfx10 $vgpr3, $vgpr8, $vgpr7, $vgpr5, $vgpr4, $vgpr6, $vgpr0, $vgpr2, $vgpr2, $sgpr0_sgpr1_sgpr2_sgpr3_sgpr4_sgpr5_sgpr6_sgpr7, $sgpr8_sgpr9_sgpr10_sgpr11, 15, 2, 0, 0, 0, 0, 0, 0, 0, implicit $exec  :: (load (s128))
 ...
+
+---
+name: kill
+tracksRegLiveness: true
+body: |
+  bb.0:
+    liveins: $sgpr0_sgpr1, $sgpr4
+    ; CHECK-LABEL: name: kill
+    ; CHECK: liveins: $sgpr0_sgpr1, $sgpr4
+    ; CHECK: BUNDLE implicit-def $sgpr2, implicit-def $sgpr2_lo16, implicit-def $sgpr2_hi16, implicit-def $sgpr3, implicit-def $sgpr3_lo16, implicit-def $sgpr3_hi16, implicit $sgpr0_sgpr1, implicit undef $sgpr4 {
+    ; CHECK:   S_CLAUSE 1
+    ; CHECK:   $sgpr2 = S_LOAD_DWORD_IMM $sgpr0_sgpr1, 0, 0
+    ; CHECK:   KILL undef renamable $sgpr4
+    ; CHECK:   $sgpr3 = S_LOAD_DWORD_IMM $sgpr0_sgpr1, 4, 0
+    ; CHECK: }
+    $sgpr2 = S_LOAD_DWORD_IMM $sgpr0_sgpr1, 0, 0
+    KILL undef renamable $sgpr4
+    $sgpr3 = S_LOAD_DWORD_IMM $sgpr0_sgpr1, 4, 0
+...