[PATCH] D73482: [AMDGPU] Fix lowering a16 image intrinsics

[Sebastian Neubauer via Phabricator via llvm-commits]

sebastian-ne added inline comments.


================
Comment at: llvm/test/CodeGen/AMDGPU/llvm.amdgcn.image.a16.encode.ll:3
+; RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck -check-prefixes=GFX9 %s
+
+define amdgpu_ps <4 x float> @load_3d(<8 x i32> inreg %rsrc, <4 x i32> inreg %smpl, float %sf, float %tf) {
----------------
arsenm wrote:
> if this is an encoding test, it should use -show-mc-encoding?
Maybe the name was chosen uncarefully, this test should ensure that all components are actually converted and packed into one register.

Before this patch, a16 is not working because the y component just gets ignored, the compiler only ensures that the x component is present.
Still, all other a16 tests are passing. So I tried to introduce this test as a regression test.
It should make sure that actually all components are "encoded" into the packed coordinates.

I guess renaming it to llvm.amdgcn.image.a16.pack.ll would make more sense?


================
Comment at: llvm/test/CodeGen/AMDGPU/llvm.amdgcn.image.gather4.a16.dim.ll:21
 ; GCN: image_gather4 v[0:3], v[0:1], s[0:7], s[8:11] dmask:0x1 a16 da{{$}}
-define amdgpu_ps <4 x float> @gather4_2darray(<8 x i32> inreg %rsrc, <4 x i32> inreg %samp, half %s, half %t, half %slice) {
 main_body:
----------------
arsenm wrote:
> These tests changes seem independent, but I think having the separate scalars has more value for testing the packing code actually works
Well, before this patch the packing did not work but the tests passed ;)
To test the packing, I added the new test.

nhaehnle said the amdgpu_ps calling convention is not build to handle f16 arguments, so it is not clear if they are packed or not?
The llvm.amdgcn.image.a16.dim.ll test, which takes i16 instead of halfs, also uses packed arguments. It groups all arguments into <2 x i16>s.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73482