Representing buffer descriptors in the AMDGPU target - call for suggestions

Code Generation AMDGPU
61

Yeah, migration to new intrinsics is definitely an option. I see basically two paths:

  1. Migrate to new intrinsics
  2. Multi-step approach:
    1. Overload the existing intrinsics, without argmem and nocapture and implement addrspace(7) lowering.
    2. Migrate frontends to native load/store/etc. where possible.
    3. Remove the overloading from the existing intrinsics, tighten to argmem and nocapture generally.

The questions are:

  1. Which approach is easier?
  2. Are there significant differences in terms of strength of alias analysis?

For the second question, where will these intrinsics be present in the IR? If they’re only present for a short time (between lowering of addrspace(7) pointers and instruction selection), then perhaps the weaker attributes just don’t matter too much.

I suppose there are also structured buffer intrinsics which are present for longer because we can’t represent them with native load/store. At least the proposal doesn’t regress them; and frontends could insert stronger attributes at call sites for those.

So I think either path is okay. We don’t have that many buffer intrinsics, so perhaps just duplicating them is for the best, but I don’t feel too strongly about it. For image intrinsics, the story would be a bit different IMO, because there are just so damn many of them.

62

I’m seeing address space 7 as a somewhat separate effort from the intrinsic conversion, since address space 7 only works for raw buffers due to limitations in what getelementptr does.

63

Sure, it can be done separately, and I did mention structured buffers. I’m just trying to keep sight of the larger picture.

64

Since it’s looking like we’ll want a new version of the intrinsics, what’re folks thoughts on having the new ones be s/buffer/buffer.ptr/, that is, we have, for example, the old intrinsic

amdgcn.raw.buffer.load.*(<4 x i32>, ...)

(deprecated, probably tossing up a warning in a release or so) and the new

amdgcn.raw.buffer.ptr.load.*(ptr addrspace(8), ...)

and an early pass (not sure exactly how early we can put it) that converts the old ones to the new ones, since that can be done losslessly.

65

Sounds fine to me, no need to bikeshed too much :wink:

66

And here’s the patch that adds the new intrinsics (and updates or duplicates all the tests) ⚙ D147547 [AMDGPU] Add buffer intrinsics that take resources as pointers