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

Wed Mar 8 02:19:25 PST 2023

kzhuravl accepted this revision.
kzhuravl added a comment.
This revision is now accepted and ready to land.

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

================
Comment at: llvm/test/CodeGen/AMDGPU/tid-kd-xnack-any.ll:3
+; 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
+
----------------
kerbowa wrote:
> kzhuravl wrote:
> > kerbowa wrote:
> > > foad wrote:
> > > > Could you pipe the binary into `llvm-readelf --notes -` instead of `llvm-objdump`, and then match text instead of hex dumps?
> > > 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.
> > What was the differfence in the kernel descriptor between passing and failing case?
> 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.
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?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145401/new/

https://reviews.llvm.org/D145401