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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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"
----------------
sheredom wrote:
> 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.
I don't follow this. Of course it's invalid to store a different value and expect a different one to be there afterwards. You seem to be implying getelementptr is invalid to use at all. The getelementptr is producing a new 128-bit value, it isn't packing the 32-bit offset into some merge value


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

https://reviews.llvm.org/D58957