[PATCH] D101620: [AMDGPU][Disassembler] Adjust img instruction address field if a16 present

[David Stuttard via Phabricator via llvm-commits]

dstuttard added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:737
+    if (BaseOpcode->Gradients) {
+      if (IsA16 || BaseOpcode->G16)
+        AddrSize += alignTo<2>(Dim->NumGradients / 2);
----------------
dp wrote:
> sebastian-ne wrote:
> > dp wrote:
> > > dp wrote:
> > > > Your change is definitely a great improvement, a lot of sp3-based MIMG tests now pass. However I'm not sure if a16 should affect size of gradients. gfx10_shader_programming only says that gradients are packed for g16 opcodes. sp3 code does not pack gradients for a16=1 either.
> > > > 
> > > > When I remove this condition from your patch, I see some improvements in test pass rate for _d and _cd opcodes. Below are a few tests which fail with your patch but pass if IsA16 condition is removed:
> > > > 
> > > >     image_sample_cd v[5:6], v[1:8], s[8:15], s[12:15] dmask:0x3 dim:SQ_RSRC_IMG_2D a16
> > > >     image_sample_cd v[5:6], v[1:8], s[8:15], s[12:15] dmask:0x3 dim:SQ_RSRC_IMG_CUBE a16
> > > >     image_sample_cd v[5:6], v[1:8], s[8:15], s[12:15] dmask:0x3 dim:SQ_RSRC_IMG_2D_ARRAY a16
> > > > 
> > > My previous comment was intended for your change in parser, sorry. https://reviews.llvm.org/D101619
> > The code in SIISelLowering suggests that A16 always needs G16 if derivatives are specified explicitely: https://github.com/llvm/llvm-project/blob/8e211bf1c895a31b3e9f49014b5494d8e1dabcf6/llvm/lib/Target/AMDGPU/SIISelLowering.cpp#L6098-L6103
> > I remember something like A16 implies G16, but I don’t remember where that comes from.
> > 
> > IIRC sp3 often shows larger registers than are actually used. LLVM is a lot stricter there.
> > IIRC sp3 often shows larger registers than are actually used. LLVM is a lot stricter there.
> 
> It is not always true. Actually llvm may align MIMG address size to 8/16: https://github.com/llvm/llvm-project/blob/e1c729c56829d3b9502b9ac2439003f87231db50/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp#L3436
> 
> > I remember something like A16 implies G16, but I don’t remember where that comes from.
> Is this a feature of our compiler or AMD H/W? 
> 
> I have just checked that the latest sp3 do distinguish g16 and a16. Below are some examples (valid sp3 code):
> 
>     image_sample_d_g16 v[0:3], [v0, v2, v4, v6],         s[0:7], s[8:11] dmask:0xf dim:SQ_RSRC_IMG_2D    
>     image_sample_d_g16 v[0:3], [v0, v2, v4],             s[0:7], s[8:11] dmask:0xf dim:SQ_RSRC_IMG_2D a16
>     image_sample_d     v[0:3], [v0, v2, v4, v6, v8, v9], s[0:7], s[8:11] dmask:0xf dim:SQ_RSRC_IMG_2D
>     image_sample_d     v[0:3], [v0, v2, v4, v6, v8],     s[0:7], s[8:11] dmask:0xf dim:SQ_RSRC_IMG_2D a16
> 
> 
I've done some more digging and I can confirm that the A16 should not be used for gradient packing. I'll update the code in all locations and re-submit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101620