[PATCH] D144034: [AMDGPU][GFX11] Legalize and select partial NSA MIMG instructions

[Carl Ritson via Phabricator via llvm-commits]

critson added a comment.

Thank you for implementing this.
I have left a few minor comments inline.



================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:6598
+  if (UsePartialNSA) {
+    if (VAddrs.size() > NSAMaxSize)
+      VAddr = getBuildDwordsVector(DAG, DL, VAddrs, NSAMaxSize - 1);
----------------
This condition is redundant, as you already placed the same condition in UsePartialNSA?


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:6670
+  if (UsePartialNSA) {
+    if (VAddrs.size() > NSAMaxSize) {
+      ArrayRef<SDValue> NSAPart(VAddrs);
----------------
Redundant condition as above -- presumably else branch implies UseNSA, so should not need to be a duplicate of it?


================
Comment at: llvm/test/CodeGen/AMDGPU/cluster_stores.ll:462
+; GFX11-NEXT:    image_sample_d v[2:5], [v8, v9, v2, v2, v[2:3]], s[0:7], s[8:11] dmask:0xf dim:SQ_RSRC_IMG_2D
+; GFX11-NEXT:    image_sample_d v[6:9], [v10, v11, v6, v6, v[6:7]], s[0:7], s[8:11] dmask:0xf dim:SQ_RSRC_IMG_2D
 ; GFX11-NEXT:    s_waitcnt vmcnt(0)
----------------
It is interesting this has been able to reduce register pressure over GFX10.


================
Comment at: llvm/test/CodeGen/AMDGPU/llvm.amdgcn.image.sample.dim.ll:1701
+; GFX11:       ; %bb.0: ; %main_body
+; GFX11-NEXT:    image_sample_c_d_o v[0:1], [v0, v1, v2, v3, v[4:8]], s[0:7], s[8:11] dmask:0x6 dim:SQ_RSRC_IMG_2D_ARRAY
+; GFX11-NEXT:    s_waitcnt vmcnt(0)
----------------
This doesn't need NSA.
I think you need to make some tweaks to the SIShrinkInstructions to handle partial NSA form as well.


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

https://reviews.llvm.org/D144034