[PATCH] D124884: [AMDGPU] Add intrinsic llvm.amdgcn.raw.buffer.load.lds

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsAMDGPU.td:1268
+  [llvm_v4i32_ty,     // rsrc(SGPR)
+   llvm_anyptr_ty,    // LDS base offset
+   llvm_i32_ty,       // offset(VGPR/imm, included in bounds checking and swizzling)
----------------
arsenm wrote:
> This should be addrspace 3 pointers only also
It cannot be overloaded on the pointee type, infrastructure limitation. We are using llvm_anyptr_ty everywhere in such context.

If in turn we cannot use pointee types at all then this could be non overloaded pointer to void in addrspace 3.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:1199-1200
+      case Intrinsic::amdgcn_raw_buffer_load_lds:
+        Info.memVT = MVT::getVT(CI.getArgOperand(1)->getType()->
+                                    getNonOpaquePointerElementType());
+        break;
----------------
arsenm wrote:
> rampitec wrote:
> > rampitec wrote:
> > > rampitec wrote:
> > > > arsenm wrote:
> > > > > arsenm wrote:
> > > > > > rampitec wrote:
> > > > > > > arsenm wrote:
> > > > > > > > Should use the return / data type?
> > > > > > > Changed to return. What do you mean by 'use data type'?
> > > > > > You're looking at the pointer element type instead of the return / data type. i.e. we would have i8/i16/i32 return types and you don't need to look at the pointer
> > > > > I just noticed there is no return type so this is just introducing a dependency on typed pointers which is a no-go.
> > > > > 
> > > > > I don't actually see why can't we match these from the buffer intrinsic plus LDS access?
> > > > It does not return anything. This instruction does not have vdata. The only way to know the size is by looking at the overloaded LDS base pointer pointee.
> > > `LDS address = LDS_base + LDS_offset + inst_offset + (TIDinWave * 4)`
> > > 
> > > We do not have TIDinWave, certainly not after selection. Even before selection it is extremely problematic.
> > > 
> > > Why typed pointer is a no-go if that works?
> > On top of that MEM_ADDR also depends on the TID. It not the same address as a normal buffer_load would use with the same operands.
> Pointee types have been removed from the IR. If this really needs the type it would need to use an attribute on the parameter to carry it which may be new territory 
It does not really need type but it needs size. I can add immediate to the intrinsic and switch to void* for LDS base.


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

https://reviews.llvm.org/D124884