[AMDGPU] Make aperture registers 64 bit

[Pierre-vh]

Summary

Makes the SRC_(SHARED|PRIVATE)_(BASE|LIMIT) registers 64 bit instead of 32.
They're still usable as 32 bit operands by using the _LO suffix.

Preparation for D137542

[foad]

Why do we need the 32-bit variants?

[Pierre-vh]

In D137767#3918904, @foad wrote:

Why do we need the 32-bit variants?

Can't this register still be used as a 32 bit operand ? (even if it's bugged) Then we need it for proper assembly/disassembly I believe, no?

[foad]

In D137767#3918918, @Pierre-vh wrote:

In D137767#3918904, @foad wrote:

Why do we need the 32-bit variants?

Can't this register still be used as a 32 bit operand ? (even if it's bugged) Then we need it for proper assembly/disassembly I believe, no?

OK, seems reasonable. I see we already have some asm/dis tests for 64-bit uses of these sources, e.g. llvm/test/MC/AMDGPU/literals.s tests s_and_b64 s[0:1], s[0:1], src_shared_base. How did that work? Should it work differently now?

[arsenm]

I do remember struggling to define a register without sub registers, and it eventually blowing up the number of defined register classes

[arsenm]

I'd rather not define a separate 32-bit version, other than _LO and _HI are probably necessary. Do we already have assembler tests for this?

[Pierre-vh]

Comments

I wasn't able to do this without creating the 16 bit subregisters.
I added a FIXME for it. If I don't add them and try to just define the
32 bit register directly using SIReg, it creates a lot of issues with register pressure sets and classes.
We end up with a bunch of autogenerated ones that are useless and just cause crashes everywhere.

[Pierre-vh]

In D137767#3919054, @foad wrote:

In D137767#3918918, @Pierre-vh wrote:

In D137767#3918904, @foad wrote:

Why do we need the 32-bit variants?

Can't this register still be used as a 32 bit operand ? (even if it's bugged) Then we need it for proper assembly/disassembly I believe, no?

OK, seems reasonable. I see we already have some asm/dis tests for 64-bit uses of these sources, e.g. llvm/test/MC/AMDGPU/literals.s tests s_and_b64 s[0:1], s[0:1], src_shared_base. How did that work? Should it work differently now?

I think the end result is the same but internally it's different. Before, it looks like SRC_SHARED_BASE was used for both 64 bit and 32 bit operand. Maybe as a hack? decodeSpecialReg64 was returning those aperture registers even though they're 32 bits.
I'm not sure how to test this properly, I think existing tests are enough but the fact that they didn't need changes makes me doubt they're covering this properly

[Pierre-vh]

clang-format

[Pierre-vh]

Note: I can't use COPY in D137542 because otherwise it'll "simplify" it and we end up with instructions that use the _HI register variants - which shouldn't be there and are just here because TableGen needs them/I get hundreds of crashes without it. It seems to assume there'll be sub1 for every reg in the class.

[arsenm]

In D137767#3927154, @Pierre-vh wrote:

Note: I can't use COPY in D137542 because otherwise it'll "simplify" it and we end up with instructions that use the _HI register variants - which shouldn't be there and are just here because TableGen needs them/I get hundreds of crashes without it. It seems to assume there'll be sub1 for every reg in the class.

That either means we're missing a class restriction or need a reserve

[Pierre-vh]

In D137767#3927784, @arsenm wrote:

In D137767#3927154, @Pierre-vh wrote:

Note: I can't use COPY in D137542 because otherwise it'll "simplify" it and we end up with instructions that use the _HI register variants - which shouldn't be there and are just here because TableGen needs them/I get hundreds of crashes without it. It seems to assume there'll be sub1 for every reg in the class.

That either means we're missing a class restriction or need a reserve

Ah the class restriction was it! I fixed it. I got confused and was using SREG_64 (includes aperture register) for the COPY dst register, but if I use SGPR (doesn't include it) then copy coalescing won't mess it up

[arsenm]

In D137767#3929898, @Pierre-vh wrote:

In D137767#3927784, @arsenm wrote:

In D137767#3927154, @Pierre-vh wrote:

Note: I can't use COPY in D137542 because otherwise it'll "simplify" it and we end up with instructions that use the _HI register variants - which shouldn't be there and are just here because TableGen needs them/I get hundreds of crashes without it. It seems to assume there'll be sub1 for every reg in the class.

That either means we're missing a class restriction or need a reserve

Ah the class restriction was it! I fixed it. I got confused and was using SREG_64 (includes aperture register) for the COPY dst register, but if I use SGPR (doesn't include it) then copy coalescing won't mess it up

But naturally it should be SReg_64. It should be valid to copy to VCC, and used directly as a SSrc_b64/VSrc_b64 operand. You may want an SReg_64 variant that excludes these

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp

567

The whole point of reserveRegisterTuples is you don't need to handle each individual subregister

[Pierre-vh]

In D137767#3931078, @arsenm wrote:

In D137767#3929898, @Pierre-vh wrote:

In D137767#3927784, @arsenm wrote:

In D137767#3927154, @Pierre-vh wrote:

Note: I can't use COPY in D137542 because otherwise it'll "simplify" it and we end up with instructions that use the _HI register variants - which shouldn't be there and are just here because TableGen needs them/I get hundreds of crashes without it. It seems to assume there'll be sub1 for every reg in the class.

That either means we're missing a class restriction or need a reserve

Ah the class restriction was it! I fixed it. I got confused and was using SREG_64 (includes aperture register) for the COPY dst register, but if I use SGPR (doesn't include it) then copy coalescing won't mess it up

But naturally it should be SReg_64. It should be valid to copy to VCC, and used directly as a SSrc_b64/VSrc_b64 operand. You may want an SReg_64 variant that excludes these

In D137542 you said I shouldn't set a register class. If I add a register class, how will it help? Should I add the new class & keep using the constrain?

[Pierre-vh]

Fix reserveRegisterTuples call

[arsenm]

In D137767#3933143, @Pierre-vh wrote:

In D137542 you said I shouldn't set a register class. If I add a register class, how will it help? Should I add the new class & keep using the constrain?

The complaint was mostly about where you set the class on the virtual register, during legalize and not during the select of the copy. Here I mean you may want a different register class to represent the use restriction where you want everything else, but to exclude the aperture regs

llvm/lib/Target/AMDGPU/AMDGPUResourceUsageAnalysis.cpp

for (const MachineInstr &MI : MBB) {
         case AMDGPU::EXEC:
         case AMDGPU::EXEC_LO:
         case AMDGPU::EXEC_HI:
         case AMDGPU::SCC:
         case AMDGPU::M0:
         case AMDGPU::M0_LO16:
         case AMDGPU::M0_HI16:
         case AMDGPU::SRC_SHARED_BASE:
+        case AMDGPU::SRC_SHARED_BASE64:
         case AMDGPU::SRC_SHARED_LIMIT:
+        case AMDGPU::SRC_SHARED_LIMIT64:
         case AMDGPU::SRC_PRIVATE_BASE:
+        case AMDGPU::SRC_PRIVATE_BASE64:
         case AMDGPU::SRC_PRIVATE_LIMIT:
+        case AMDGPU::SRC_PRIVATE_LIMIT64:
         case AMDGPU::SGPR_NULL:
         case AMDGPU::SGPR_NULL64:
         case AMDGPU::MODE:
           continue;
 
         case AMDGPU::SRC_POPS_EXITING_WAVE_ID:
           llvm_unreachable("src_pops_exiting_wave_id should not be used");
 

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp

BitVector SIRegisterInfo::getReservedRegs(const MachineFunction &MF) const {
   reserveRegisterTuples(Reserved, AMDGPU::M0);
 
   // Reserve src_vccz, src_execz, src_scc.
   reserveRegisterTuples(Reserved, AMDGPU::SRC_VCCZ);
   reserveRegisterTuples(Reserved, AMDGPU::SRC_EXECZ);
   reserveRegisterTuples(Reserved, AMDGPU::SRC_SCC);
 
   // Reserve the memory aperture registers.
   reserveRegisterTuples(Reserved, AMDGPU::SRC_SHARED_BASE);
+  reserveRegisterTuples(Reserved, AMDGPU::SRC_SHARED_BASE64);
   reserveRegisterTuples(Reserved, AMDGPU::SRC_SHARED_LIMIT);
+  reserveRegisterTuples(Reserved, AMDGPU::SRC_SHARED_LIMIT64);
   reserveRegisterTuples(Reserved, AMDGPU::SRC_PRIVATE_BASE);
+  reserveRegisterTuples(Reserved, AMDGPU::SRC_PRIVATE_BASE64);
   reserveRegisterTuples(Reserved, AMDGPU::SRC_PRIVATE_LIMIT);
+  reserveRegisterTuples(Reserved, AMDGPU::SRC_PRIVATE_LIMIT64);
 
   // Reserve src_pops_exiting_wave_id - support is not implemented in Codegen.
   reserveRegisterTuples(Reserved, AMDGPU::SRC_POPS_EXITING_WAVE_ID);
 
   // Reserve xnack_mask registers - support is not implemented in Codegen.
   reserveRegisterTuples(Reserved, AMDGPU::XNACK_MASK);
 
   // Reserve lds_direct register - support is not implemented in Codegen.

llvm/lib/Target/AMDGPU/SIRegisterInfo.td

 def SCC : SIReg<"scc">;
 
 // Encoding changes between subtarget generations.
 // See also Utils/AMDGPUBaseInfo.cpp MAP_REG2REG.
 defm M0_gfxpre11 : SIRegLoHi16 <"m0", 124>;
 defm M0_gfx11plus : SIRegLoHi16 <"m0", 125>;
 defm M0 : SIRegLoHi16 <"m0", 0>;
 
-defm SGPR_NULL_gfxpre11 : SIRegLoHi16 <"null", 125>;
-defm SGPR_NULL_gfx11plus : SIRegLoHi16 <"null", 124>;
+multiclass Constant32and64BitSGPR<string name, bits<16> regIdx> {
   let isConstant = true in {
-defm SGPR_NULL : SIRegLoHi16 <"null", 0>;
-defm SGPR_NULL_HI : SIRegLoHi16 <"", 0>;
-} // isConstant = true
+    defm "" : SIRegLoHi16 <name, regIdx>;
+    defm _HI : SIRegLoHi16 <"", regIdx>;
 
-def SGPR_NULL64 :
-    RegisterWithSubRegs<"null", [SGPR_NULL, SGPR_NULL_HI]> {
+    def "64" : RegisterWithSubRegs<name, [!cast<Register>(NAME), !cast<Register>(NAME#_HI)]> {
       let Namespace = "AMDGPU";
       let SubRegIndices = [sub0, sub1];
-  let HWEncoding = SGPR_NULL.HWEncoding;
-  let isConstant = true;
+      let HWEncoding = !cast<Register>(NAME).HWEncoding;
     }
-
-let isConstant = true in {
-defm SRC_SHARED_BASE : SIRegLoHi16<"src_shared_base", 235>;
-defm SRC_SHARED_LIMIT : SIRegLoHi16<"src_shared_limit", 236>;
-defm SRC_PRIVATE_BASE : SIRegLoHi16<"src_private_base", 237>;
-defm SRC_PRIVATE_LIMIT : SIRegLoHi16<"src_private_limit", 238>;
   } // isConstant = true
+}
+
+defm SGPR_NULL_gfxpre11 : SIRegLoHi16 <"null", 125>;
+defm SGPR_NULL_gfx11plus : SIRegLoHi16 <"null", 124>;
+
+defm SGPR_NULL : Constant32and64BitSGPR<"null", 0>;
+
+defm SRC_SHARED_BASE : Constant32and64BitSGPR<"src_shared_base", 235>;
+defm SRC_SHARED_LIMIT : Constant32and64BitSGPR<"src_shared_limit", 236>;
+defm SRC_PRIVATE_BASE : Constant32and64BitSGPR<"src_private_base", 237>;
+defm SRC_PRIVATE_LIMIT : Constant32and64BitSGPR<"src_private_limit", 238>;
+
 defm SRC_POPS_EXITING_WAVE_ID : SIRegLoHi16<"src_pops_exiting_wave_id", 239>;
 
 // Not addressable
 def MODE : SIReg <"mode", 0>;
 
 def LDS_DIRECT : SIReg <"src_lds_direct", 254> {
   // There is no physical register corresponding to this. This is an
   // encoding value in a source field, which will ultimately trigger a

 }
 
 let GeneratePressureSet = 0, HasSGPR = 1 in {
 // Subset of SReg_32 without M0 for SMRD instructions and alike.
 // See comments in SIInstructions.td for more info.
 def SReg_32_XM0_XEXEC : SIRegisterClass<"AMDGPU", [i32, f32, i16, f16, v2i16, v2f16, i1], 32,
   (add SGPR_32, VCC_LO, VCC_HI, FLAT_SCR_LO, FLAT_SCR_HI, XNACK_MASK_LO, XNACK_MASK_HI,
    SGPR_NULL, SGPR_NULL_HI, TTMP_32, TMA_LO, TMA_HI, TBA_LO, TBA_HI, SRC_SHARED_BASE,
-   SRC_SHARED_LIMIT, SRC_PRIVATE_BASE, SRC_PRIVATE_LIMIT, SRC_POPS_EXITING_WAVE_ID,
+   SRC_SHARED_BASE_HI, SRC_SHARED_LIMIT, SRC_SHARED_LIMIT_HI, SRC_PRIVATE_BASE,
+   SRC_PRIVATE_BASE_HI, SRC_PRIVATE_LIMIT, SRC_PRIVATE_LIMIT_HI, SRC_POPS_EXITING_WAVE_ID,
    SRC_VCCZ, SRC_EXECZ, SRC_SCC)> {
   let AllocationPriority = 0;
 }
 
 def SReg_LO16_XM0_XEXEC : SIRegisterClass<"AMDGPU", [i16, f16], 16,
   (add SGPR_LO16, VCC_LO_LO16, VCC_HI_LO16, FLAT_SCR_LO_LO16, FLAT_SCR_HI_LO16,
    XNACK_MASK_LO_LO16, XNACK_MASK_HI_LO16, SGPR_NULL_LO16, SGPR_NULL_HI_LO16, TTMP_LO16,
    TMA_LO_LO16, TMA_HI_LO16, TBA_LO_LO16, TBA_HI_LO16, SRC_SHARED_BASE_LO16,
    SRC_SHARED_LIMIT_LO16, SRC_PRIVATE_BASE_LO16, SRC_PRIVATE_LIMIT_LO16,
-   SRC_POPS_EXITING_WAVE_ID_LO16, SRC_VCCZ_LO16, SRC_EXECZ_LO16, SRC_SCC_LO16)> {
+   SRC_SHARED_BASE_HI_LO16, SRC_SHARED_LIMIT_HI_LO16, SRC_PRIVATE_BASE_HI_LO16,
+   SRC_PRIVATE_LIMIT_HI_LO16, SRC_POPS_EXITING_WAVE_ID_LO16, SRC_VCCZ_LO16, SRC_EXECZ_LO16,
+   SRC_SCC_LO16)> {
   let Size = 16;
   let AllocationPriority = 0;
 }
 
 def SReg_32_XEXEC_HI : SIRegisterClass<"AMDGPU", [i32, f32, i16, f16, v2i16, v2f16, i1], 32,
   (add SReg_32_XM0_XEXEC, EXEC_LO, M0_CLASS)> {
   let AllocationPriority = 0;
 }

 
 def TTMP_64 : SIRegisterClass<"AMDGPU", [v2i32, i64, f64, v4i16, v4f16], 32,
                             (add TTMP_64Regs)> {
   let isAllocatable = 0;
   let HasSGPR = 1;
 }
 
 def SReg_64_XEXEC : SIRegisterClass<"AMDGPU", [v2i32, i64, v2f32, f64, i1, v4i16, v4f16], 32,
-  (add SGPR_64, VCC, FLAT_SCR, XNACK_MASK, SGPR_NULL64, TTMP_64, TBA, TMA)> {
+  (add SGPR_64, VCC, FLAT_SCR, XNACK_MASK, SGPR_NULL64, SRC_SHARED_BASE64,
+       SRC_SHARED_LIMIT64, SRC_PRIVATE_BASE64, SRC_PRIVATE_LIMIT64, TTMP_64, TBA, TMA)> {
   let CopyCost = 1;
   let AllocationPriority = 1;
   let HasSGPR = 1;
 }
 
 def SReg_64 : SIRegisterClass<"AMDGPU", [v2i32, i64, v2f32, f64, i1, v4i16, v4f16], 32,
   (add SReg_64_XEXEC, EXEC)> {
   let CopyCost = 1;