[AMDGPU] Reserve extra SGPR blocks wth XNACK "any" TID Setting

[kerbowa]

Summary

ASMPrinter was relying on feature bits to setup extra SGRPs in the knerel
descriptor for the xnack_mask. This was broken for the dynamic XNACK "any" TID
setting which could cause user SGPRs to be clobbered if the number of SGPRs
reserved was near a granulated block boundary.

When XNACK was enabled this worked correctly in the ASMParser which meant some
kernels were only failing without "-save-temps".

Fixes: SWDEV-382764

[foad]

llvm/test/CodeGen/AMDGPU/tid-kd-xnack-any.ll

3

Could you pipe the binary into llvm-readelf --notes - instead of llvm-objdump, and then match text instead of hex dumps?

[kerbowa]

llvm/test/CodeGen/AMDGPU/tid-kd-xnack-any.ll

3

The notes section contains metadata that may not match the kernel descriptor (KD) in .rodata. In a failed test, the metadata was the same in both passing and failing cases, but the KD was different. I can add an extra runline though.

[kerbowa]

Add readelf run-line to test.

[kzhuravl]

llvm/test/CodeGen/AMDGPU/tid-kd-xnack-any.ll

3

What was the differfence in the kernel descriptor between passing and failing case?

[kerbowa]

llvm/test/CodeGen/AMDGPU/tid-kd-xnack-any.ll

3

The difference was the granulated SGPR field. It was 1 in the failing case, 2 in the passing version. I can't really replicate it here since the only reason it was passing with save-temps, is that the ASM parser was correctly checking the dynamic target ID setting and encoding the correct kernel descriptor. We already have MC tests for that. The metadata and what the ASM printer is doing has always been incorrect.

[kzhuravl]

LGTM, but please add a todo or a fixme (see comment below), thanks

llvm/test/CodeGen/AMDGPU/tid-kd-xnack-any.ll

3

There are several internal discussions related to asm directives and I think we are going to end up adding the assembler directive for granulated sgpr count. Once that directive is added we can change this test from checking hex to checking that directory. Would you mind adding a TODO or a FIXME saying this test needs to be updated to use the directive instead of hex once the directive is available?

[kerbowa]

Actually, this is breaking tests with non-HSA. Is TargetID relevant for pal/graphics/ect @foad, or should the default there be XNACK- in the absence of any explicit subtarget features being added?

[kerbowa]

Update tests.

[kerbowa]

Added AMDGPU group to reviewers.

Is there any objection to changing the defaults for subtargets that support XNACK to always reserve extra SGPRs unless -xnack is explicitly requested? This would impact graphics as well. The old defaults were doing the opposite and only reserving the extra SGPRs with +xnack, meaning the default in the absence of either +/-xnack will be changing.

llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp

void AMDGPUAsmPrinter::emitFunctionBodyEnd() {
   const GCNSubtarget &STM = MF->getSubtarget<GCNSubtarget>();
 
   SmallString<128> KernelName;
   getNameWithPrefix(KernelName, &MF->getFunction());
   getTargetStreamer()->EmitAmdhsaKernelDescriptor(
       STM, KernelName, getAmdhsaKernelDescriptor(*MF, CurrentProgramInfo),
       CurrentProgramInfo.NumVGPRsForWavesPerEU,
       CurrentProgramInfo.NumSGPRsForWavesPerEU -
-          IsaInfo::getNumExtraSGPRs(&STM,
-                                    CurrentProgramInfo.VCCUsed,
-                                    CurrentProgramInfo.FlatUsed),
+          IsaInfo::getNumExtraSGPRs(
+              &STM, CurrentProgramInfo.VCCUsed, CurrentProgramInfo.FlatUsed,
+              getTargetStreamer()->getTargetID()->isXnackOnOrAny()),
       CurrentProgramInfo.VCCUsed, CurrentProgramInfo.FlatUsed,
       CodeObjectVersion);
 
   Streamer.popSection();
 }
 
 void AMDGPUAsmPrinter::emitFunctionEntryLabel() {
   if (TM.getTargetTriple().getOS() == Triple::AMDHSA &&
void AMDGPUAsmPrinter::getSIProgramInfo(SIProgramInfo &ProgInfo,
   }
 
   const SIMachineFunctionInfo *MFI = MF.getInfo<SIMachineFunctionInfo>();
 
   // The calculations related to SGPR/VGPR blocks are
   // duplicated in part in AMDGPUAsmParser::calculateGPRBlocks, and could be
   // unified.
   unsigned ExtraSGPRs = IsaInfo::getNumExtraSGPRs(
-      &STM, ProgInfo.VCCUsed, ProgInfo.FlatUsed);
+      &STM, ProgInfo.VCCUsed, ProgInfo.FlatUsed,
+      getTargetStreamer()->getTargetID()->isXnackOnOrAny());
 
   // Check the addressable register limit before we add ExtraSGPRs.
   if (STM.getGeneration() >= AMDGPUSubtarget::VOLCANIC_ISLANDS &&
       !STM.hasSGPRInitBug()) {
     unsigned MaxAddressableNumSGPRs = STM.getAddressableNumSGPRs();
     if (ProgInfo.NumSGPR > MaxAddressableNumSGPRs) {
       // This can happen due to a compiler bug or when using inline asm.
       LLVMContext &Ctx = MF.getFunction().getContext();

llvm/test/CodeGen/AMDGPU/tid-kd-xnack-any.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx90a < %s | FileCheck --check-prefixes=ASM %s
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx90a --filetype=obj < %s | llvm-objdump -s -j .rodata - | FileCheck --check-prefixes=OBJ %s
+
+define amdgpu_kernel void @kern() {
+; ASM-LABEL: kern:
+; ASM: .amdhsa_next_free_sgpr 5
+; ASM: .amdhsa_reserve_xnack_mask 1
+
+; Verify that an extra SGPR block is reserved with XNACK "any" tid setting.
+; OBJ: Contents of section .rodata:
+; OBJ-NEXT: 0000 00000000 00000000 00000000 00000000  ................
+; OBJ-NEXT: 0010 00000000 00000000 00000000 00000000  ................
+; OBJ-NEXT: 0020 00000000 00000000 00000000 00000000  ................
+; OBJ-NEXT: 0030 4000af00 88000000 01000000 00000000  @...............
+entry:
+  tail call void asm sideeffect "", "~{s[0:4]}"()
+  ret void
+}
+
+!llvm.module.flags = !{!0}
+!0 = !{i32 1, !"amdgpu_code_object_version", i32 400}

llvm/test/CodeGen/AMDGPU/tid-kd-xnack-off.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx90a -mattr=-xnack < %s | FileCheck --check-prefixes=ASM %s
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx90a -mattr=-xnack --filetype=obj < %s | llvm-objdump -s -j .rodata - | FileCheck --check-prefixes=OBJ %s
+
+define amdgpu_kernel void @kern() {
+; ASM-LABEL: kern:
+; ASM: .amdhsa_next_free_sgpr 5
+; ASM: .amdhsa_reserve_xnack_mask 0
+
+; Verify that an extra SGPR block is not reserved with XNACK "off" tid setting.
+; OBJ: Contents of section .rodata:
+; OBJ-NEXT: 0000 00000000 00000000 00000000 00000000  ................
+; OBJ-NEXT: 0010 00000000 00000000 00000000 00000000  ................
+; OBJ-NEXT: 0020 00000000 00000000 00000000 00000000  ................
+; OBJ-NEXT: 0030 0000af00 88000000 01000000 00000000  ................
+entry:
+  tail call void asm sideeffect "", "~{s[0:4]}"()
+  ret void
+}
+
+!llvm.module.flags = !{!0}
+!0 = !{i32 1, !"amdgpu_code_object_version", i32 400}

llvm/test/CodeGen/AMDGPU/tid-kd-xnack-on.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx90a -mattr=+xnack < %s | FileCheck --check-prefixes=ASM %s
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx90a -mattr=+xnack --filetype=obj < %s | llvm-objdump -s -j .rodata - | FileCheck --check-prefixes=OBJ %s
+
+define amdgpu_kernel void @kern() {
+; ASM-LABEL: kern:
+; ASM: .amdhsa_next_free_sgpr 5
+; ASM: .amdhsa_reserve_xnack_mask 1
+
+; Verify that an extra SGPR block is reserved with XNACK "on" tid setting.
+; OBJ: Contents of section .rodata:
+; OBJ-NEXT: 0000 00000000 00000000 00000000 00000000  ................
+; OBJ-NEXT: 0010 00000000 00000000 00000000 00000000  ................
+; OBJ-NEXT: 0020 00000000 00000000 00000000 00000000  ................
+; OBJ-NEXT: 0030 4000af00 88000000 01000000 00000000  @...............
+entry:
+  tail call void asm sideeffect "", "~{s[0:4]}"()
+  ret void
+}
+
+!llvm.module.flags = !{!0}
+!0 = !{i32 1, !"amdgpu_code_object_version", i32 400}