[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:5-16
+; GFX9-LABEL: load_3d:
+; GFX9:       ; %bb.0: ; %main_body
+; GFX9-NEXT:    s_mov_b64 s[12:13], exec
+; GFX9-NEXT:    s_wqm_b64 exec, exec
+; GFX9-NEXT:    v_cvt_f16_f32_e32 v0, v0
+; GFX9-NEXT:    v_cvt_f16_f32_e32 v1, v1
+; GFX9-NEXT:    v_and_b32_e32 v0, 0xffff, v0
----------------
rtaylor wrote:
> Do you need all this checking or can you just check the image_sample instruction? If so, it would be cleaner and easier to read and easier to tell what you are checking  with just the GFX9: image_sample... 
Testing the image_sample instruction is not enough.
In the previous tests, only the image_sample was tested and the tests passed. However, a16 was not working because packing the f16 arguments into the registers did work only partially.
So the previous tests were incomplete.
This is exactly the part that this test fills. The significant part of this test is not the image_sample, but the conversion and packing of all arguments into the registers.
It tests the part that went wrong before this patch.

Is the intent of this test clear? I can try to explain it more detailed.

With my updated patch, the other tests check more details, so we could also remove this small test. Any opinions on this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73482