[PATCH] D125117: [AMDGPU][GFX10] Support base+soffset+offset SMEM loads.

[Ivan Kosarev via Phabricator via llvm-commits]

kosarev added a comment.

In D125117#3500499 <https://reviews.llvm.org/D125117#3500499>, @foad wrote:

> To clarify, you are deliberately only doing this for SMEM loads, not for stores or probes or any other SMEM instruction types? Given that, the patch looks OK to me, but I would appreciate a second opinion maybe from @dp.

Yes, that's just for soffset+offset loads. Feels like that's enough for a single patch.



================
Comment at: llvm/lib/Target/AMDGPU/SMInstructions.td:751
 
-  let Inst{7-0}   = !if(ps.has_offset, offset{7-0}, ?);
-  let Inst{8}     = imm;
+  let Inst{7-0}   = !if(ps.has_offset, offset{7-0}, !if(ps.has_soffset, soffset{7-0}, ?));
+  let Inst{8}     = ps.has_offset;
----------------
foad wrote:
> Can you just `soffset` instead of `soffset{7-0}`, here and elsewhere?
Sure, done.


================
Comment at: llvm/lib/Target/AMDGPU/SMInstructions.td:911-913
+  let Inst{52-32} = !if(ps.has_offset, offset{20-0}, !if(ps.has_soffset, 0, ?));
+  let Inst{63-57} = !if(ps.has_soffset, soffset{6-0},
+                        !if(ps.has_offset, !cast<int>(SGPR_NULL.HWEncoding), ?));
----------------
foad wrote:
> These expressions seem over-complicated. Is it really important to set these fields to `?` in the "!has_offset && !has_soffset" case, which presumably will never happen? Can you simplify to:
> ```
>   let Inst{52-32} = !if(ps.has_offset, offset{20-0}, 0);
>   let Inst{63-57} = !if(ps.has_soffset, soffset{6-0}, !cast<int>(SGPR_NULL.HWEncoding));
> ```
There are actually SMEMs that do not employ the fields, e.g., `s_dcache_inv` and `s_memrealtime`. So yes, I think we need the `?`s. Added a comment explaining that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125117