This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Define data layout entries for buffers
ClosedPublic

Authored by krzysz00 on Mar 6 2023, 3:23 PM.

Details

Summary

Per discussion at
https://discourse.llvm.org/t/representing-buffer-descriptors-in-the-amdgpu-target-call-for-suggestions/68798,
we define two new address spaces for AMDGCN targets.

The first is address space 7, a non-integral address space (which was
already in the data layout) that has 160-bit pointers (which are
256-bit aligned) and uses a 32-bit offset. These pointers combine a
128-bit buffer descriptor and a 32-bit offset, and will be usable with
normal LLVM operations (load, store, GEP). However, they will be
rewritten out of existence before code generation.

The second of these is address space 8, the address space for "buffer
resources". These will be used to represent the resource arguments to
buffer instructions, and new buffer intrinsics will be defined that
take them instead of <4 x i32> as resource arguments. ptr
addrspace(8). These pointers are 128-bits long (with the same
alignment). They must not be used as the arguments to getelementptr or
otherwise used in address computations, since they can have
arbitrarily complex inherent addressing semantics that can't be
represented in LLVM. Even though, like their address space 7 cousins,
these pointers have deterministic ptrtoint/inttoptr semantics, they
are defined to be non-integral in order to prevent optimizations that
rely on pointers being a [0, [addr_max]] value from applying to them.

Future work includes:

  • Defining new buffer intrinsics that take ptr addrspace(8) resources.
  • A late rewrite to turn address space 7 operations into buffer

intrinsics and offset computations.

This commit also updates the "fallback address space" for buffer
intrinsics to the buffer resource, and updates the alias analysis
table.

Depends on D143437

Diff Detail

Event Timeline

krzysz00 created this revision.Mar 6 2023, 3:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2023, 3:23 PM
krzysz00 requested review of this revision.Mar 6 2023, 3:23 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 6 2023, 3:23 PM
foad added a comment.Mar 7 2023, 1:51 AM

Just my 2p: it feels a bit premature to commit patches for this. It feels more like something you could prototype on a branch somewhere and come back when you have more experience with how it all works out in practice.

But I don't actually object to the patch, if the other reviewers are happy and it doens't break anything.

The first is address space 7, a non-integral address space (which was
already in the data layout) that has 160-bit pointers (which are
256-bit aligned)

Any particular reason for choosing 256-bit alignment?

However, they must not be used as the arguments to
getelementptr or otherwise used in address computations

I don't understand what kind of rule this is and how it would be enforced. Is it something that will be written into the IR LangRef?

This commit also updates the "fallback address space" for buffer
intrinsics to the buffer resource,

It's not clear to me that this is any more or less correct, since 7 and 8 behave identically wrt alias analysis don't they?

@foad I was trying to avoid sending in one mega-patch that has the entire prototype.

As to your comments:

  • Why 256-bit alignment? It's the next power of 2, and using an alignment that isn't a power of 2 causes an assertion failure
  • Re not being allowed to use address space 8 pointers in the usual LLVM operations, I'd call that a target-specific rule about what address space 8 means. I figure this is something that we can check in some sort of AMDGPU-specific validation
  • I changed the fallback address space because the plan is that those intrinsics will be taking address space 8 arguments in the future
nhaehnle added a comment.EditedMar 7 2023, 12:08 PM

@foad I was trying to avoid sending in one mega-patch that has the entire prototype.

The best way to do this is to work on a branch that you rebase regularly. You can push the branch to a personal clone of llvm-project on GitHub and point folks there from e.g. Discourse, and for the purpose of Phabricator you can set up a "Stack" via the "Edit Related Revisions" option.

If you're using arc, then git rebase -i -x "arc diff @^" is one way to update a bunch of patches at once. There are probably other tools that people have written.

These [addrspace(8) pointers] are, however, integral, since inttoptr and ptrtoint behave deterministically and reasonably.

That doesn't make sense to me. I thought "integral" means that

inttoptr(ptrtoint(p) + offset)

is the same (in terms of address, and potentially modulo provenance questions) as

gep i8, p, offset

But that is rather untrue for addrspace(8), isn't it?

llvm/docs/AMDGPUUsage.rst
795

Typo: experimental

805–806

Presumably it is only permitted if the underlying descriptor satisfies all the rules mentioned for addrspace(7).

krzysz00 updated this revision to Diff 507869.Mar 23 2023, 1:47 PM

Fix typo, rebase while making next patch

krzysz00 updated this revision to Diff 507870.Mar 23 2023, 1:50 PM

... actually fix the typo

krzysz00 updated this revision to Diff 509761.Mar 30 2023, 11:58 AM
krzysz00 edited the summary of this revision. (Show Details)

Rebase for splitting s.buffer.load effect change into earlier patch

krzysz00 updated this revision to Diff 510056.Mar 31 2023, 9:20 AM
krzysz00 edited the summary of this revision. (Show Details)

Per discussion on the s.buffer.load revision, don't make those changes, and so revert this patch back to what it used to be.

krzysz00 updated this revision to Diff 510848.Apr 4 2023, 10:20 AM

Rebase data layout changes

krzysz00 updated this revision to Diff 512488.Apr 11 2023, 9:12 AM

Rebase, since we have addrspace 128

krzysz00 updated this revision to Diff 514232.Apr 17 2023, 7:50 AM

Fix new merge conflicts

krzysz00 updated this revision to Diff 516953.Apr 25 2023, 3:18 PM
krzysz00 edited the summary of this revision. (Show Details)

Make address space 8 non-integral as well because that's a good idea and we'll have an address space cast intrinsic.

arsenm accepted this revision.May 1 2023, 1:26 PM

LGTM. It may not be the end step and may be annoying to make further changes from this point, but on its own this isn't worse than before

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
998

Should this todo be dropped?

This revision is now accepted and ready to land.May 1 2023, 1:26 PM
krzysz00 updated this revision to Diff 518843.May 2 2023, 1:19 PM

Rebase to handle new legalization tests.

This revision was landed with ongoing or failed builds.May 3 2023, 8:26 AM
This revision was automatically updated to reflect the committed changes.
jplehr added a subscriber: jplehr.May 3 2023, 9:10 AM

Hey Krzysztof,
I believe this broke the openmp offload buildbot https://lab.llvm.org/buildbot/#/builders/193/builds/30576