[PATCH] D125279: [AMDGPU] Add llvm.amdgcn.global.load.lds intrinsic

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsAMDGPU.td:1789
+   llvm_i32_ty,                        // Data byte size: 1/2/4
+   llvm_i32_ty,                        // imm offset
+   llvm_i32_ty],                       // auxiliary data (imm, cachepolicy (bit 0 = glc/sc0,
----------------
arsenm wrote:
> Does this really need the offset parameter? It's not a buffer intrinsic with the funny different swizzling behavior between scalar and vector, so can't we just match this from the base pointer?
This has 2 separate offsets. VGPR offset is applied to the global memory only, while immediate offset applies to both global and LDS. To have it matching first pointer shall be (g_ptr + const) and the second (l_ptr + const) which we cannot guarantee to preserve.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:1314-1315
+    Info.memVT = EVT::getIntegerVT(CI.getContext(), Width * 8);
+    Info.flags |= MachineMemOperand::MOLoad | MachineMemOperand::MOStore |
+                  MachineMemOperand::MOVolatile;
+    return true;
----------------
arsenm wrote:
> I guess this API just isn't strong enough to handle 2 MMOs
Yes, so it is only for the selection time and later I split it.


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

https://reviews.llvm.org/D125279