[AMDGPU] Tidy SReg/SGPR definitions using template class

[critson]

Summary

Use a multiclass to consistently define SReg/SGPR/TTMP register classes.
Add missing TTMP registers for 96b, 160b, 192b, 224b.

[foad]

Looks like a nice refactoring.

llvm/lib/Target/AMDGPU/SIInstructions.td

1242–1243 ↗

(On Diff #357861)

Was SGPR_96 some kind of anomaly among the existing classes?

llvm/lib/Target/AMDGPU/SIRegisterInfo.td

725

Don't you want to divide by two rounding up, so !sra(!add(numRegs, 1), 1)?

740

Should we be defining TTMP classes for all sizes instead of working around it like this?

[arsenm]

llvm/lib/Target/AMDGPU/SIInstructions.td

1242–1243 ↗

(On Diff #357861)

I think the *GPR_* class names are better where applicable

llvm/lib/Target/AMDGPU/SIRegisterInfo.td

746

Typo inherence

[critson]

• Address reviewer comments

[critson]

llvm/lib/Target/AMDGPU/SIInstructions.td

1242–1243 ↗

(On Diff #357861)

OK, I've put these back. Pretty much everything else in this file uses SReg_*

llvm/lib/Target/AMDGPU/SIRegisterInfo.td

725

Thanks.

740

At the moment if I create the TTMP_96Regs set then SGPR_128 stops working correctly. So I need to investigate further if we want to do that. Also SGPR_1024 will never have an associated TTMP set because there are only 16 trap registers.

[critson]

In D105800#2871152, @foad wrote:

Should we be defining TTMP classes for all sizes instead of working around it like this?

I don't think we can at present.

If I define TTMP_96Regs this implicitly causes TTMP_128_with_sub0_sub1_sub2 to be created.
This then creates SReg_128_with_sub0_sub1_sub2.
Now SGPR_128_with_sub0_sub1_sub2 inherits from SReg_128_with_sub0_sub1_sub2 instead of from SGPR_128.
(The superclass of SReg_128_with_sub0_sub1_sub2 is SReg_128, but the superclasses of SGPR_128_with_sub0_sub1_sub2 are SReg_128, SGPR_128, SReg_128_with_sub0_sub1_sub2.)
The net result is that SGPR_128_with_sub0_sub1_sub2 is marked isAllocatable = 0, causing allocation new failures.

The bottom line is that I don't think isAllocatable = 0 currently supports what we are trying to do.
Best guess is it is only appropriate for isolated or leaf register classes.
I can pursue a TableGen change to try and address this, but it would be good to get the immediate issue of allocation failures with SGPR_192 fixed.

llvm/lib/Target/AMDGPU/SIRegisterInfo.td

740

Should we be defining TTMP classes for all sizes instead of working around it like this?

[critson]

• Rework based on TableGen changes in D105967
• Define all applicable TTMP register classes

[foad]

Looks good, just nits inline.

llvm/lib/Target/AMDGPU/SIRegisterInfo.td

724

Wouldn't it be a bit simpler to use [] as the default, and test it with !empty?

725

Isn't defvar a better way of defining all these variables, instead of making them optional arguments that are never actually used?

[critson]

llvm/lib/Target/AMDGPU/SIRegisterInfo.td

724

It's not possible to initialise a SIRegisterTuples variable with [], so this is the simplest solution in my opinion. Beyond creating a dummy SIRegisterTuples variable to use.

725

Good point. It seems we don't actually use defvar much in AMDGPU.

[critson]

• Address nits
• Fix allocation priority of SReg_224

llvm/lib/Target/AMDGPU/SIRegisterInfo.td

 def SGPR_1024Regs : SIRegisterTuples<getSubRegs<32>.ret, SGPR_32, 105, 4, 32, "s">;
 
 // Trap handler TMP 32-bit registers
 def TTMP_32 : RegisterClass<"AMDGPU", [i32, f32, v2i16, v2f16], 32,
                             (add (sequence "TTMP%u", 0, 15))> {
   let isAllocatable = 0;
 }
 
+// Trap handler TMP 16-bit registers
 def TTMP_LO16 : RegisterClass<"AMDGPU", [i16, f16], 16,
                               (add (sequence "TTMP%u_LO16", 0, 15))> {
   let Size = 16;
   let isAllocatable = 0;
 }
 
 // Trap handler TMP 64-bit registers
 def TTMP_64Regs : SIRegisterTuples<getSubRegs<2>.ret, TTMP_32, 15, 2, 2, "ttmp">;
 
+// Trap handler TMP 96-bit registers
+def TTMP_96Regs : SIRegisterTuples<getSubRegs<3>.ret, TTMP_32, 15, 3, 3, "ttmp">;
+
 // Trap handler TMP 128-bit registers
 def TTMP_128Regs : SIRegisterTuples<getSubRegs<4>.ret, TTMP_32, 15, 4, 4, "ttmp">;
 
+// Trap handler TMP 160-bit registers
+def TTMP_160Regs : SIRegisterTuples<getSubRegs<5>.ret, TTMP_32, 15, 4, 5, "ttmp">;
+
+// Trap handler TMP 192-bit registers
+def TTMP_192Regs : SIRegisterTuples<getSubRegs<6>.ret, TTMP_32, 15, 4, 6, "ttmp">;
+
+// Trap handler TMP 224-bit registers
+def TTMP_224Regs : SIRegisterTuples<getSubRegs<7>.ret, TTMP_32, 15, 4, 7, "ttmp">;
+
+// Trap handler TMP 256-bit registers
 def TTMP_256Regs : SIRegisterTuples<getSubRegs<8>.ret, TTMP_32, 15, 4, 8, "ttmp">;
 
+// Trap handler TMP 512-bit registers
 def TTMP_512Regs : SIRegisterTuples<getSubRegs<16>.ret, TTMP_32, 15, 4, 16, "ttmp">;
 
 class TmpRegTuplesBase<int index, int size,
                        list<Register> subRegs,
                        list<SubRegIndex> indices = getSubRegs<size>.ret,
                        int index1 = !add(index, size, -1),
                        string name = "ttmp["#index#":"#index1#"]"> :
   RegisterWithSubRegs<name, subRegs> {

 }
 
 def SReg_1 : RegisterClass<"AMDGPU", [i1], 32,
   (add SReg_1_XEXEC, EXEC, EXEC_LO)> {
   let CopyCost = 1;
   let isAllocatable = 0;
 }
 
-// Requires 2 s_mov_b64 to copy
-let CopyCost = 2 in {
-
-// There are no 3-component scalar instructions, but this is needed
-// for symmetry with VGPRs.
-def SGPR_96 : RegisterClass<"AMDGPU", [v3i32, v3f32], 32,
-  (add SGPR_96Regs)> {
-  let AllocationPriority = 14;
-}
-
-def SReg_96 : RegisterClass<"AMDGPU", [v3i32, v3f32], 32,
-  (add SGPR_96)> {
-  let AllocationPriority = 14;
-}
-
-def SGPR_128 : RegisterClass<"AMDGPU", [v4i32, v4f32, v2i64], 32,
-                             (add SGPR_128Regs)> {
-  let AllocationPriority = 15;
-}
-
-def TTMP_128 : RegisterClass<"AMDGPU", [v4i32, v4f32, v2i64], 32,
-                             (add TTMP_128Regs)> {
-  let isAllocatable = 0;
-}
-
-def SReg_128 : RegisterClass<"AMDGPU", [v4i32, v4f32, v2i64, v2f64], 32,
-                             (add SGPR_128, TTMP_128)> {
-  let isAllocatable = 0;
-}
-
-} // End CopyCost = 2
-
-// There are no 5-component scalar instructions, but this is needed
-// for symmetry with VGPRs.
-def SGPR_160 : RegisterClass<"AMDGPU", [v5i32, v5f32], 32,
-                             (add SGPR_160Regs)> {
-  let AllocationPriority = 16;
-}
-
-def SReg_160 : RegisterClass<"AMDGPU", [v5i32, v5f32], 32,
-                             (add SGPR_160)> {
-  // FIXME: Should be isAllocatable = 0, but that causes all TableGen-generated
-  // subclasses of SGPR_160 to be marked unallocatable too.
-  // This occurs because SGPR_160 and SReg_160 classes are equivalent in size
-  // meaning their enumeration order is dependent on alphanumeric ordering of
-  // their names.  The superclass for inherence is the last one in topological
-  // order (i.e. enumeration order), hence SReg_160 is selected.
-  // Potential workarounds involve renaming SGPR_160, adding another class
-  // which is ordered last and hence used for inheritance, or adding more
-  // registers to SReg_160 to cause it to be moved earlier in the superclass
-  // list.
-  let CopyCost = 3;
-}
-
-// There are no 6-component scalar instructions, but this is needed
-// for symmetry with VGPRs.
-def SGPR_192 : RegisterClass<"AMDGPU", [v6i32, v6f32, v3i64, v3f64], 32, (add SGPR_192Regs)> {
-  let AllocationPriority = 17;
-}
-
-def SReg_192 : RegisterClass<"AMDGPU", [v6i32, v6f32, v3i64, v3f64], 32, (add SGPR_192)> {
-  let isAllocatable = 0;
-  let CopyCost = 3;
-}
+multiclass SRegClass<int numRegs, int priority,
+                     list<ValueType> regTypes,
+                     SIRegisterTuples regList,
+                     SIRegisterTuples ttmpList = regList,
+                     int copyCost = !sra(!add(numRegs, 1), 1)> {
+  defvar hasTTMP = !ne(regList, ttmpList);
+  defvar suffix = !cast<string>(!mul(numRegs, 32));
+  defvar sgprName = !strconcat("SGPR_", suffix);
+  defvar ttmpName = !strconcat("TTMP_", suffix);
 
-// There are no 7-component scalar instructions, but this is needed
-// for symmetry with VGPRs.
-def SGPR_224 : RegisterClass<"AMDGPU", [v7i32, v7f32], 32, (add SGPR_224Regs)> {
-  let AllocationPriority = 18;
+  let AllocationPriority = priority, CopyCost = copyCost in {
+    def "" # sgprName : RegisterClass<"AMDGPU", regTypes, 32, (add regList)> {
     }
 
-def SReg_224 : RegisterClass<"AMDGPU", [v7i32, v7f32], 32, (add SGPR_224)> {
+    if hasTTMP then {
+      def "" # ttmpName : RegisterClass<"AMDGPU", regTypes, 32, (add ttmpList)> {
         let isAllocatable = 0;
-  let CopyCost = 4;
-}
-
-def SGPR_256 : RegisterClass<"AMDGPU", [v8i32, v8f32, v4i64, v4f64], 32, (add SGPR_256Regs)> {
-  let AllocationPriority = 19;
       }
-
-def TTMP_256 : RegisterClass<"AMDGPU", [v8i32, v8f32, v4i64, v4f64], 32, (add TTMP_256Regs)> {
-  let isAllocatable = 0;
     }
 
-def SReg_256 : RegisterClass<"AMDGPU", [v8i32, v8f32, v4i64, v4f64], 32,
-                             (add SGPR_256, TTMP_256)> {
-  // Requires 4 s_mov_b64 to copy
-  let CopyCost = 4;
+    def SReg_ # suffix :
+      RegisterClass<"AMDGPU", regTypes, 32,
+                    !con(!dag(add, [!cast<RegisterClass>(sgprName)], ["sgpr"]),
+                    !if(hasTTMP,
+                        !dag(add, [!cast<RegisterClass>(ttmpName)], ["ttmp"]),
+                        (add)))> {
       let isAllocatable = 0;
     }
-
-def SGPR_512 : RegisterClass<"AMDGPU", [v16i32, v16f32, v8i64, v8f64], 32,
-                             (add SGPR_512Regs)> {
-  let AllocationPriority = 20;
   }
-
-def TTMP_512 : RegisterClass<"AMDGPU", [v16i32, v16f32, v8i64, v8f64], 32,
-                             (add TTMP_512Regs)> {
-  let isAllocatable = 0;
 }
 
-def SReg_512 : RegisterClass<"AMDGPU", [v16i32, v16f32, v8i64, v8f64], 32,
-                             (add SGPR_512, TTMP_512)> {
-  // Requires 8 s_mov_b64 to copy
-  let CopyCost = 8;
-  let isAllocatable = 0;
-}
+defm "" : SRegClass<3, 14, [v3i32, v3f32], SGPR_96Regs, TTMP_96Regs>;
+defm "" : SRegClass<4, 15, [v4i32, v4f32, v2i64], SGPR_128Regs, TTMP_128Regs>;
+defm "" : SRegClass<5, 16, [v5i32, v5f32], SGPR_160Regs, TTMP_160Regs>;
+defm "" : SRegClass<6, 17, [v6i32, v6f32, v3i64, v3f64], SGPR_192Regs, TTMP_192Regs>;
+defm "" : SRegClass<7, 18, [v7i32, v7f32], SGPR_224Regs, TTMP_224Regs>;
+defm "" : SRegClass<8, 19, [v8i32, v8f32, v4i64, v4f64], SGPR_256Regs, TTMP_256Regs>;
+defm "" : SRegClass<16, 20, [v16i32, v16f32, v8i64, v8f64], SGPR_512Regs, TTMP_512Regs>;
+defm "" : SRegClass<32, 21, [v32i32, v32f32, v16i64, v16f64], SGPR_1024Regs>;
 
 def VRegOrLds_32 : RegisterClass<"AMDGPU", [i32, f32, i16, f16, v2i16, v2f16], 32,
                                  (add VGPR_32, LDS_DIRECT_CLASS)> {
   let isAllocatable = 0;
 }
 
-def SGPR_1024 : RegisterClass<"AMDGPU", [v32i32, v32f32, v16i64, v16f64], 32,
-                              (add SGPR_1024Regs)> {
-  let AllocationPriority = 21;
-}
-
-def SReg_1024 : RegisterClass<"AMDGPU", [v32i32, v32f32, v16i64, v16f64], 32,
-                              (add SGPR_1024)> {
-  let CopyCost = 16;
-  let isAllocatable = 0;
-}
-
 // Register class for all vector registers (VGPRs + Interpolation Registers)
 class VRegClassBase<int numRegs, list<ValueType> regTypes, dag regList> :
     RegisterClass<"AMDGPU", regTypes, 32, regList> {
   let Size = !mul(numRegs, 32);
 
   // Requires n v_mov_b32 to copy
   let CopyCost = numRegs;
   let AllocationPriority = numRegs;

llvm/test/CodeGen/AMDGPU/inline-asm.i128.ll

 ; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 ; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx908 -stop-after=finalize-isel -o - %s | FileCheck -check-prefix=GFX908 %s
 ; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx90a -stop-after=finalize-isel -o - %s | FileCheck -check-prefix=GFX90A %s
 
 ; Make sure we only use one 128-bit register instead of 2 for i128 asm
 ; constraints
 
 define amdgpu_kernel void @s_input_output_i128() {
   ; GFX908-LABEL: name: s_input_output_i128
   ; GFX908: bb.0 (%ir-block.0):
-  ; GFX908:   INLINEASM &"; def $0", 1 /* sideeffect attdialect */, 4784138 /* regdef:SGPR_128 */, def %4
+  ; GFX908:   INLINEASM &"; def $0", 1 /* sideeffect attdialect */, 5111818 /* regdef:SGPR_128 */, def %4
   ; GFX908:   [[COPY:%[0-9]+]]:sgpr_128 = COPY %4
-  ; GFX908:   INLINEASM &"; use $0", 1 /* sideeffect attdialect */, 4784137 /* reguse:SGPR_128 */, [[COPY]]
+  ; GFX908:   INLINEASM &"; use $0", 1 /* sideeffect attdialect */, 5111817 /* reguse:SGPR_128 */, [[COPY]]
   ; GFX908:   S_ENDPGM 0
   ; GFX90A-LABEL: name: s_input_output_i128
   ; GFX90A: bb.0 (%ir-block.0):
-  ; GFX90A:   INLINEASM &"; def $0", 1 /* sideeffect attdialect */, 4784138 /* regdef:SGPR_128 */, def %4
+  ; GFX90A:   INLINEASM &"; def $0", 1 /* sideeffect attdialect */, 5111818 /* regdef:SGPR_128 */, def %4
   ; GFX90A:   [[COPY:%[0-9]+]]:sgpr_128 = COPY %4
-  ; GFX90A:   INLINEASM &"; use $0", 1 /* sideeffect attdialect */, 4784137 /* reguse:SGPR_128 */, [[COPY]]
+  ; GFX90A:   INLINEASM &"; use $0", 1 /* sideeffect attdialect */, 5111817 /* reguse:SGPR_128 */, [[COPY]]
   ; GFX90A:   S_ENDPGM 0
   %val = tail call i128 asm sideeffect "; def $0", "=s"()
   call void asm sideeffect "; use $0", "s"(i128 %val)
   ret void
 }
 
 define amdgpu_kernel void @v_input_output_i128() {
   ; GFX908-LABEL: name: v_input_output_i128
   ; GFX908: bb.0 (%ir-block.0):
-  ; GFX908:   INLINEASM &"; def $0", 1 /* sideeffect attdialect */, 4390922 /* regdef:VReg_128 */, def %4
+  ; GFX908:   INLINEASM &"; def $0", 1 /* sideeffect attdialect */, 4718602 /* regdef:VReg_128 */, def %4
   ; GFX908:   [[COPY:%[0-9]+]]:vreg_128 = COPY %4
-  ; GFX908:   INLINEASM &"; use $0", 1 /* sideeffect attdialect */, 4390921 /* reguse:VReg_128 */, [[COPY]]
+  ; GFX908:   INLINEASM &"; use $0", 1 /* sideeffect attdialect */, 4718601 /* reguse:VReg_128 */, [[COPY]]
   ; GFX908:   S_ENDPGM 0
   ; GFX90A-LABEL: name: v_input_output_i128
   ; GFX90A: bb.0 (%ir-block.0):
-  ; GFX90A:   INLINEASM &"; def $0", 1 /* sideeffect attdialect */, 4521994 /* regdef:VReg_128_Align2 */, def %4
+  ; GFX90A:   INLINEASM &"; def $0", 1 /* sideeffect attdialect */, 4849674 /* regdef:VReg_128_Align2 */, def %4
   ; GFX90A:   [[COPY:%[0-9]+]]:vreg_128_align2 = COPY %4
-  ; GFX90A:   INLINEASM &"; use $0", 1 /* sideeffect attdialect */, 4521993 /* reguse:VReg_128_Align2 */, [[COPY]]
+  ; GFX90A:   INLINEASM &"; use $0", 1 /* sideeffect attdialect */, 4849673 /* reguse:VReg_128_Align2 */, [[COPY]]
   ; GFX90A:   S_ENDPGM 0
   %val = tail call i128 asm sideeffect "; def $0", "=v"()
   call void asm sideeffect "; use $0", "v"(i128 %val)
   ret void
 }
 
 define amdgpu_kernel void @a_input_output_i128() {
   ; GFX908-LABEL: name: a_input_output_i128
   ; GFX908: bb.0 (%ir-block.0):
-  ; GFX908:   INLINEASM &"; def $0", 1 /* sideeffect attdialect */, 4325386 /* regdef:AReg_128 */, def %4
+  ; GFX908:   INLINEASM &"; def $0", 1 /* sideeffect attdialect */, 4653066 /* regdef:AReg_128 */, def %4
   ; GFX908:   [[COPY:%[0-9]+]]:areg_128 = COPY %4
-  ; GFX908:   INLINEASM &"; use $0", 1 /* sideeffect attdialect */, 4325385 /* reguse:AReg_128 */, [[COPY]]
+  ; GFX908:   INLINEASM &"; use $0", 1 /* sideeffect attdialect */, 4653065 /* reguse:AReg_128 */, [[COPY]]
   ; GFX908:   S_ENDPGM 0
   ; GFX90A-LABEL: name: a_input_output_i128
   ; GFX90A: bb.0 (%ir-block.0):
-  ; GFX90A:   INLINEASM &"; def $0", 1 /* sideeffect attdialect */, 4456458 /* regdef:AReg_128_Align2 */, def %4
+  ; GFX90A:   INLINEASM &"; def $0", 1 /* sideeffect attdialect */, 4784138 /* regdef:AReg_128_Align2 */, def %4
   ; GFX90A:   [[COPY:%[0-9]+]]:areg_128_align2 = COPY %4
-  ; GFX90A:   INLINEASM &"; use $0", 1 /* sideeffect attdialect */, 4456457 /* reguse:AReg_128_Align2 */, [[COPY]]
+  ; GFX90A:   INLINEASM &"; use $0", 1 /* sideeffect attdialect */, 4784137 /* reguse:AReg_128_Align2 */, [[COPY]]
   ; GFX90A:   S_ENDPGM 0
   %val = call i128 asm sideeffect "; def $0", "=a"()
   call void asm sideeffect "; use $0", "a"(i128 %val)
   ret void
 }