[PATCH] D58957: [AMDGPU] Add an experimental buffer fat pointer address space.

[Neil Henning via Phabricator via llvm-commits]

sheredom marked an inline comment as done.
sheredom added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:297
+  // flat, non-integral buffer fat pointers.
     return "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32"
          "-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128"
----------------
arsenm wrote:
> sheredom wrote:
> > arsenm wrote:
> > > sheredom wrote:
> > > > arsenm wrote:
> > > > > Shouldn't this add p7:128?
> > > > It'd need to be p7:160 - but I'm entirely unsure whether LLVM will drop a lung on a non-power of 2 pointer size.
> > > Why does it need to be 160? It should be 128 like the descriptor
> > So for non-swizzable buffer descriptors we want to model it using normal LLVM load/store/atomic instructions, so that we have no intrinsics required for them at all. To model this we need a 160-bit pointer for the 128-bit descriptor + 32-bit offset. This is super important because it means these 160-bit pointers partake in all the normal load/store optimizations without us having to have special cases for whatever new intrinsics we'd have to introduce.
> > 
> > We're trying our best to avoid the need for any new intrinsics (if at all possible, it won't always be possible though).
> I still don't see how the offset is part of the pointer itself. You could always use an offset of 0, so it would be a matter of changing the GEP index type to be different from the bit width of the pointer, and an optimization during codegen to fold it in
So if we explicitly laid out the pointer as a p7:128, what would happen if you stored that pointer into memory (say an alloca)? This is fine if you are storing the original pointer (just the 128-bit memory buffer descriptor), but if you had GEP'ed into this pointer, when you store the pointer into memory you are losing the offset the information as part of that store. It seems dangerous to me that we'd pretend the pointer is 128-bit when actually for all intermediate uses of the pointer it'll contain 160-bits of valid information.

I think I'd rather just leave it as non-integral which solves all these issues perfectly well and also has the added benefit of restricting the usages of the pointer from things we don't want to support.


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

https://reviews.llvm.org/D58957