[PATCH] D124884: [AMDGPU] Add intrinsics llvm.amdgcn.{raw|struct}.buffer.load.lds

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec marked 2 inline comments as done.
rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:8260
+
+    return DAG.getMemIntrinsicNode(AMDGPUISD::BUFFER_LOAD, DL,
+                                   M->getVTList(), Ops, M->getMemoryVT(), MMO);
----------------
arsenm wrote:
> I don't see how / where this preserves the LDS bit
It has different number of operands comparing to the SIbuffer_load, so selects into _LDS versions of opcodes.

In fact after I have removed offset split because we cannot do it on one pointer only, and dropped multi-dword support I start thinking it might be better to drop SIbuffer_load_lds, patterns, and produce MachineSDNode right here (like in the D125279 for global load), it will not be so much code anymore and I will be able to produce 2 separate memory operands.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:389
+    if (DataOpIdx == -1) { // LDS DMA
+      Width = (*LdSt.memoperands_begin())->getSize();
+      return true;
----------------
arsenm wrote:
> If you're going to rely on the memory operand, the verifier needs to start enforcing these have one memory operand (well, 2 actually with the same sizes)
On a second thought it is better to just return false here. We cannot have a reasonable pointer here on either side anyway, and in fact even 2 memory operands which it should ideally have should be of a different size for a sub-dword operations. A load can be sub-dword, but the store is always extended to a dword.


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

https://reviews.llvm.org/D124884