[AMDGPU] Correct the number of SGPR blocks used for GFX9

[rochauha]

Summary

Edit : Updating the summary based on comments

Even though granularity is 8, the roundup must be an even number of 8-granules for GFX9.
Probably this also needs to be mentioned in https://llvm.org/docs/AMDGPUUsage.html#amdgpu-amdhsa-compute-pgm-rsrc1-gfx6-gfx10-table for GRANULATED_WAVEFRONT_SGPR_COUNT.

The difference is seen when a the rounded value aligns to 8 but not to 16. (for example 40, 56).
This patch corrects the roundup for GFX9, hence correcting the number of SGPRBlocks.

[arsenm]

Needs test

[foad]

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

439–441

Why have you changed this?

[rochauha]

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

439–441

To follow the computation of GRANULATED_WAVEFRONT_SGPR_COUNT for GFX9, as mentioned in https://llvm.org/docs/AMDGPUUsage.html#amdgpu-amdhsa-compute-pgm-rsrc1-gfx6-gfx10-table

[rochauha]

In D84194#2164227, @arsenm wrote:

Needs test

I think these changes are tested using the test in https://reviews.llvm.org/D80713.
In fact the issue was found when testing round tripping for the above patch. I guess this can only be verified by a round trip test when we assemble->disassemble->re-assemble? Such a test is already present in the patch for D80713.

I am not sure how else can we look at the value of GRANULATED_WAVEFRONT_SGPR_COUNT in a test case.

[scott.linder]

In D84194#2164406, @rochauha wrote:

In D84194#2164227, @arsenm wrote:

Needs test

I think these changes are tested using the test in https://reviews.llvm.org/D80713.
In fact the issue was found when testing round tripping for the above patch. I guess this can only be verified by a round trip test when we assemble->disassemble->re-assemble? Such a test is already present in the patch for D80713.

I am not sure how else can we look at the value of GRANULATED_WAVEFRONT_SGPR_COUNT in a test case.

I think you should be able to test this by adding another KD case to llvm/test/MC/AMDGPU/hsa-v3.s, and just checking the hexdump of the KD as for the other cases there. It should be pretty painless, you can just copy-paste the minimal one, set the SGPR count to trigger the bug, and update the GRANULATED_WAVEFRONT_SGPR_COUNT bits in the expected dump.

[scott.linder]

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

348

I don't know if this is actually accurate, I think the reason for the "2 *" in the equation for GFX9 is not because the allocation granule is 16. It is still 8 for gfx9, but there is an additional constraint that you must allocate an even number of granules.

It is a bit confusing, and I would like @kzhuravl to weigh in as IIRC he was who originally helped me understand this when we were updating the assembler.

439–440

If the above is true, and the granule for gfx9 is in fact 8, then I would just move all of the handling of the "even" requirement into this function, i.e. change this to:

unsigned NumSGPRBlocks = NumSGPRs / (isGFX9(*STI) ? 2 * getSGPREncodingGranule(STI) : getSGPREncodingGranule(STI)) - 1;

[foad]

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

439–440

The current patch does have the advantage that it closely matches the documentation that Ronak pointed to. Though I suppose we could update the documentation too.

[foad]

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

434–440

Incidentally the alignTo and the division could be combined into a single call to divideCeil.

[t-tye]

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

348

For GFX9 the granularity is as specified in AMDGPUUsage which is 8. As @scott.linder mentions SPI rounds up to an even number of 8-granules. From the hardware spec:

Number of SGPRS, granularity 8. SPI rounds up reg setting and allocs gran16. Range is from 0-13 allocating (SGPRS/2+1)*16: 16,16,32,32 ... 112,112

[rochauha]

Updated patch based on comments.
Updated old tests.
Added new test.

[rochauha]

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

434–440

Done.

[foad]

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

439

Don't you still need a std::max somewhere in here to cope with the NumSGPRs==0 case?

[rochauha]

Added missing std::max.

[rochauha]

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

439

Done. Thanks!

[scott.linder]

I discussed with Tony today, and I was thinking about this the wrong way.

SPI does not require the granule count to be even, it just rounds up the granule count before actually performing the allocation. This means, from the compiler's perspective, when it is calculating things like the AMDGPU::IsaInfo::getMaxNumSGPRs it must consider the "allocation" granule size (IsaInfo::getSGPRAllocGranule). Conversely, from the assembler/diassembler perspective, it must consider the "encoding" granule size (IsaInfo::getSGPREncodingGranule). It is perfectly OK to have a GFX9 code object with a granulated SGPR count of 1, and we should allow emitting that in the assembler so that the disassembler can accurately reproduce those code objects.

I don't think there is any fix needed here, we already separate these two concepts and correctly apply them elsewhere. I think I just led you astray in the disassembly patch; you should only be using the encoding granule size, and shouldn't need any special handling for e.g. GFX9 to handle the fact that the allocation and encoding granule sizes are not equal.

[rochauha]

In D84194#2170882, @scott.linder wrote:

I discussed with Tony today, and I was thinking about this the wrong way.

SPI does not require the granule count to be even, it just rounds up the granule count before actually performing the allocation. This means, from the compiler's perspective, when it is calculating things like the AMDGPU::IsaInfo::getMaxNumSGPRs it must consider the "allocation" granule size (IsaInfo::getSGPRAllocGranule). Conversely, from the assembler/diassembler perspective, it must consider the "encoding" granule size (IsaInfo::getSGPREncodingGranule). It is perfectly OK to have a GFX9 code object with a granulated SGPR count of 1, and we should allow emitting that in the assembler so that the disassembler can accurately reproduce those code objects.

I don't think there is any fix needed here, we already separate these two concepts and correctly apply them elsewhere. I think I just led you astray in the disassembly patch; you should only be using the encoding granule size, and shouldn't need any special handling for e.g. GFX9 to handle the fact that the allocation and encoding granule sizes are not equal.

Correct me if I'm wrong. So we must not take inverse of the mentioned GFX9 calculation (the one where we divide by 16 before roundup) as it is for allocation granule size? And hence the disassembly computation will be same for GFX6-8 and GFX9 (because the encoding granule size is the same)?

[scott.linder]

In D84194#2173959, @rochauha wrote:

In D84194#2170882, @scott.linder wrote:

I discussed with Tony today, and I was thinking about this the wrong way.

SPI does not require the granule count to be even, it just rounds up the granule count before actually performing the allocation. This means, from the compiler's perspective, when it is calculating things like the AMDGPU::IsaInfo::getMaxNumSGPRs it must consider the "allocation" granule size (IsaInfo::getSGPRAllocGranule). Conversely, from the assembler/diassembler perspective, it must consider the "encoding" granule size (IsaInfo::getSGPREncodingGranule). It is perfectly OK to have a GFX9 code object with a granulated SGPR count of 1, and we should allow emitting that in the assembler so that the disassembler can accurately reproduce those code objects.

I don't think there is any fix needed here, we already separate these two concepts and correctly apply them elsewhere. I think I just led you astray in the disassembly patch; you should only be using the encoding granule size, and shouldn't need any special handling for e.g. GFX9 to handle the fact that the allocation and encoding granule sizes are not equal.

Correct me if I'm wrong. So we must not take inverse of the mentioned GFX9 calculation (the one where we divide by 16 before roundup) as it is for allocation granule size? And hence the disassembly computation will be same for GFX6-8 and GFX9 (because the encoding granule size is the same)?

Correct, you can treat all hardware the same and calculate:

NumSGPRs = (NumSGPRBlocks + 1) * getSGPREncodingGranule()

I still think it might be good to make this into a function in AMDGPU::IsaInfo to be the inverse of getNumSGPRBlocks

[rochauha]

Based on comments and discussion, the difference for GFX9 is being handled using allocation granule sizes and no change is required.

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

if (Version.Major >= 10)
     return getAddressableNumSGPRs(STI);
   if (Version.Major >= 8)
     return 16;
   return 8;
 }
 
 unsigned getSGPREncodingGranule(const MCSubtargetInfo *STI) {
   return 8;
 }
 
 unsigned getTotalNumSGPRs(const MCSubtargetInfo *STI) {
   IsaVersion Version = getIsaVersion(STI->getCPU());
   if (Version.Major >= 8)
     return 800;
   return 512;
 }
 

 
 unsigned getNumExtraSGPRs(const MCSubtargetInfo *STI, bool VCCUsed,
                           bool FlatScrUsed) {
   return getNumExtraSGPRs(STI, VCCUsed, FlatScrUsed,
                           STI->getFeatureBits().test(AMDGPU::FeatureXNACK));
 }
 
 unsigned getNumSGPRBlocks(const MCSubtargetInfo *STI, unsigned NumSGPRs) {
-  NumSGPRs = alignTo(std::max(1u, NumSGPRs), getSGPREncodingGranule(STI));
+  // Even though granularity is 8, the roundup must be an even number of
+  // 8-granules for GFX9.
+  unsigned Alignment = isGFX9(*STI) ? getSGPREncodingGranule(STI) * 2
+                                    : getSGPREncodingGranule(STI);
   // SGPRBlocks is actual number of SGPR blocks minus 1.
-  return NumSGPRs / getSGPREncodingGranule(STI) - 1;
+  unsigned NumSGPRBlocks = divideCeil(NumSGPRs, Alignment) - 1;
+  return isGFX9(*STI) ? NumSGPRBlocks * 2 : NumSGPRBlocks;
 }
 
 unsigned getVGPRAllocGranule(const MCSubtargetInfo *STI,
                              Optional<bool> EnableWavefrontSize32) {
   bool IsWave32 = EnableWavefrontSize32 ?
       *EnableWavefrontSize32 :
       STI->getFeatureBits().test(FeatureWavefrontSize32);
 
   if (hasGFX10_3Insts(*STI))

llvm/test/MC/AMDGPU/hsa-v3.s

 // RUN: llvm-mc -mattr=+code-object-v3 -triple amdgcn-amd-amdhsa -mcpu=gfx904 -mattr=+xnack < %s | FileCheck --check-prefix=ASM %s
 // RUN: llvm-mc -mattr=+code-object-v3 -triple amdgcn-amd-amdhsa -mcpu=gfx904 -mattr=+xnack -filetype=obj < %s > %t
 // RUN: llvm-readelf -sections -symbols -relocations %t | FileCheck --check-prefix=READOBJ %s
 // RUN: llvm-objdump -s -j .rodata %t | FileCheck --check-prefix=OBJDUMP %s
 
 // big endian not supported
 // XFAIL: host-byteorder-big-endian
 
 // READOBJ: Section Headers
 // READOBJ: .text   PROGBITS {{[0-9a-f]+}} {{[0-9a-f]+}} {{[0-9a-f]+}} {{[0-9]+}} AX {{[0-9]+}} {{[0-9]+}} 256
-// READOBJ: .rodata PROGBITS {{[0-9a-f]+}} {{[0-9a-f]+}}        000100 {{[0-9]+}}  A {{[0-9]+}} {{[0-9]+}} 64
+// READOBJ: .rodata PROGBITS {{[0-9a-f]+}} {{[0-9a-f]+}}        000140 {{[0-9]+}}  A {{[0-9]+}} {{[0-9]+}} 64
 
 // READOBJ: Relocation section '.rela.rodata' at offset
 // READOBJ: 0000000000000010 {{[0-9a-f]+}}00000005 R_AMDGPU_REL64 0000000000000000 .text + 10
 // READOBJ: 0000000000000050 {{[0-9a-f]+}}00000005 R_AMDGPU_REL64 0000000000000000 .text + 110
 // READOBJ: 0000000000000090 {{[0-9a-f]+}}00000005 R_AMDGPU_REL64 0000000000000000 .text + 210
 // READOBJ: 00000000000000d0 {{[0-9a-f]+}}00000005 R_AMDGPU_REL64 0000000000000000 .text + 310
+// READOBJ: 0000000000000110 {{[0-9a-f]+}}00000005 R_AMDGPU_REL64 0000000000000000 .text + 410
 
 // READOBJ: Symbol table '.symtab' contains {{[0-9]+}} entries:
 // READOBJ: {{[0-9]+}}: 0000000000000100  0 FUNC    LOCAL  PROTECTED 2 complete
 // READOBJ: {{[0-9]+}}: 0000000000000040 64 OBJECT  LOCAL  DEFAULT   3 complete.kd
 // READOBJ: {{[0-9]+}}: 0000000000000300  0 FUNC    LOCAL  PROTECTED 2 disabled_user_sgpr
 // READOBJ: {{[0-9]+}}: 00000000000000c0 64 OBJECT  LOCAL  DEFAULT   3 disabled_user_sgpr.kd
+// READOBJ: {{[0-9]+}}: 0000000000000400  0 FUNC    LOCAL  PROTECTED 2 gfx9_sgpr
+// READOBJ: {{[0-9]+}}: 0000000000000100 64 OBJECT  LOCAL  DEFAULT   3 gfx9_sgpr.kd
 // READOBJ: {{[0-9]+}}: 0000000000000000  0 FUNC    LOCAL  PROTECTED 2 minimal
 // READOBJ: {{[0-9]+}}: 0000000000000000 64 OBJECT  LOCAL  DEFAULT   3 minimal.kd
 // READOBJ: {{[0-9]+}}: 0000000000000200  0 FUNC    LOCAL  PROTECTED 2 special_sgpr
 // READOBJ: {{[0-9]+}}: 0000000000000080 64 OBJECT  LOCAL  DEFAULT   3 special_sgpr.kd
 
 // OBJDUMP: Contents of section .rodata
 // Note, relocation for KERNEL_CODE_ENTRY_BYTE_OFFSET is not resolved here.
 // minimal
 // OBJDUMP-NEXT: 0000 00000000 00000000 00000000 00000000
 // OBJDUMP-NEXT: 0010 00000000 00000000 00000000 00000000
 // OBJDUMP-NEXT: 0020 00000000 00000000 00000000 00000000
 // OBJDUMP-NEXT: 0030 0000ac00 80000000 00000000 00000000
 // complete
 // OBJDUMP-NEXT: 0040 01000000 01000000 00000000 00000000
 // OBJDUMP-NEXT: 0050 00000000 00000000 00000000 00000000
 // OBJDUMP-NEXT: 0060 00000000 00000000 00000000 00000000
-// OBJDUMP-NEXT: 0070 c2500104 1f0f007f 7f000000 00000000
+// OBJDUMP-NEXT: 0070 82500104 1f0f007f 7f000000 00000000
 // special_sgpr
 // OBJDUMP-NEXT: 0080 00000000 00000000 00000000 00000000
 // OBJDUMP-NEXT: 0090 00000000 00000000 00000000 00000000
 // OBJDUMP-NEXT: 00a0 00000000 00000000 00000000 00000000
 // OBJDUMP-NEXT: 00b0 00010000 80000000 00000000 00000000
 // disabled_user_sgpr
 // OBJDUMP-NEXT: 00c0 00000000 00000000 00000000 00000000
 // OBJDUMP-NEXT: 00d0 00000000 00000000 00000000 00000000
 // OBJDUMP-NEXT: 00e0 00000000 00000000 00000000 00000000
 // OBJDUMP-NEXT: 00f0 0000ac00 80000000 00000000 00000000
+// gfx9_sgpr
+// OBJDUMP-NEXT: 0100 00000000 00000000 00000000 00000000
+// OBJDUMP-NEXT: 0110 00000000 00000000 00000000 00000000
+// OBJDUMP-NEXT: 0120 00000000 00000000 00000000 00000000
+// OBJDUMP-NEXT: 0130 0001ac00 80000000 00000000 00000000
+
 
 .text
 // ASM: .text
 
 .amdgcn_target "amdgcn-amd-amdhsa--gfx904+xnack"
 // ASM: .amdgcn_target "amdgcn-amd-amdhsa--gfx904+xnack"
 
 .p2align 8

 special_sgpr:
   s_endpgm
 
 .p2align 8
 .type disabled_user_sgpr,@function
 disabled_user_sgpr:
   s_endpgm
 
+.p2align 8
+.type gfx9_sgpr,@function
+gfx9_sgpr:
+  s_endpgm
+
+
 .rodata
 // ASM: .rodata
 
 // Test that only specifying required directives is allowed, and that defaulted
 // values are omitted.
 .p2align 6
 .amdhsa_kernel minimal
   .amdhsa_next_free_vgpr 0
.amdhsa_kernel disabled_user_sgpr
   .amdhsa_next_free_sgpr 0
 .end_amdhsa_kernel
 
 // ASM: .amdhsa_kernel disabled_user_sgpr
 // ASM: .amdhsa_next_free_vgpr 0
 // ASM-NEXT: .amdhsa_next_free_sgpr 0
 // ASM: .end_amdhsa_kernel
 
+// Test GRANULATED_WAVEFRONT_SGPR_COUNT for GFX9
+.p2align 6
+.amdhsa_kernel gfx9_sgpr
+  .amdhsa_next_free_vgpr 0
+  .amdhsa_next_free_sgpr 33
+.end_amdhsa_kernel
+
+// ASM: .amdhsa_kernel gfx9_sgpr
+// ASM: .amdhsa_next_free_vgpr 0
+// ASM-NEXT: .amdhsa_next_free_sgpr 33
+// ASM: .end_amdhsa_kernel
+
+
 .section .foo
 
 .byte .amdgcn.gfx_generation_number
 // ASM: .byte 9
 
 .byte .amdgcn.gfx_generation_minor
 // ASM: .byte 0