This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][NFC]: Emit metadata for hidden_heap_v1 kernarg
ClosedPublic

Authored by cfang on Feb 4 2022, 11:42 AM.

Diff Detail

Event Timeline

cfang created this revision.Feb 4 2022, 11:42 AM
cfang requested review of this revision.Feb 4 2022, 11:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2022, 11:42 AM
Herald added a subscriber: wdng. · View Herald Transcript
cfang updated this revision to Diff 409300.Feb 16 2022, 9:36 AM

We are now using the similar approach as hostcall handling (https://reviews.llvm.org/D119216).
We search through the call-graph for implictarg_ptr + offset for the use of hidden_heap_v1 kernarg.
And emit metadata for it only when we find at one such cases.

b-sumner added inline comments.Feb 16 2022, 1:20 PM
llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
521

We could potentially be more specific with KernargLoc instead of MemoryLoc.

sameerds added inline comments.Feb 17 2022, 1:06 AM
llvm/lib/Target/AMDGPU/AMDGPUAttributes.def
22

It seems we need to document these attributes in AMDGPUUsage, which I missed for no-hostcall-ptr.

https://llvm.org/docs/AMDGPUUsage.html#llvm-ir-attributes

llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
521

Or more importantly, the outer function needs a more specific name ... it is checking whether the supplied offset is being accessed from the implicitarg_ptr base.

cfang added inline comments.Feb 19 2022, 11:22 AM
llvm/lib/Target/AMDGPU/AMDGPUAttributes.def
22

Can you suggest the description for both no-hostcall and no-heap-ptr? Thanks.

llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
521

Will update as suggested. Thanks.

521

Can we use "funcRetrievesImplicitKernarg(...) ?

sameerds added inline comments.Feb 20 2022, 5:28 AM
llvm/lib/Target/AMDGPU/AMDGPUAttributes.def
22

How about this:

"Similar to amdgpu-no-implicitarg-ptr, except specific to the implicit kernel argument that holds the pointer to the hostcall buffer. If this attribute is absent, then the amdgpu-no-implicitarg-ptr is also removed."

I do believe the removal already happens, but it will be good to convince ourselves about that.

llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
521

Sounds good to me. But "kernarg" usually refers to the segment ... maybe say "KernelArgument" instead? I am okay with either, since the function has a very limited scope.

cfang updated this revision to Diff 410338.Feb 21 2022, 10:59 AM

Add descriptions of amdgpu-no-hostcall-ptr and amdgpu-no-heap-ptr in the docs.
Update a couple of function names.

cfang marked 5 inline comments as done.Feb 21 2022, 11:03 AM
cfang added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUAttributes.def
22

Agreed! Thanks for the suggestions.

sameerds added inline comments.Feb 21 2022, 9:01 PM
llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
413

Why was this removed? Is there some other place which guarantees the same relationship?

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h
57

The first letter should be capital. \returns is not the beginning of the sentence ... doxygen will render only the stuff that follows the keyword.

cfang marked an inline comment as done.Feb 21 2022, 9:12 PM
cfang added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
413

I think the explicit call of amdgcn_implicitarg_ptr will guarantee IMPLICIT_ARG_PTR.

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h
57

I see. Thanks for pointing out.

cfang updated this revision to Diff 410443.Feb 21 2022, 9:26 PM

Updated based on Sameer's comments.

sameerds added inline comments.Feb 21 2022, 10:13 PM
llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
413

That sounds correct. But then this should be asserted somewhere ... probably in the code that manifests the attribute.

cfang added inline comments.Feb 21 2022, 10:29 PM
llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
413

Do you think the for loop a few lines ahead in the same function should and "MUST" catch amdgcn_implicitarg_ptr? So that we don't need an assert?

386: for (Function *Callee : AAEdges.getOptimisticEdges()) {

sameerds added inline comments.Feb 21 2022, 10:39 PM
llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
413

Maybe it does. But that's not the point. This relationship between the two attributes is pretty important and we should assert. Even the coding standard says so [1] and it is pretty much a life-saver. Maybe I should have asserted myself, but that last change was kinda in a bit of a hurry!

[1] https://llvm.org/docs/CodingStandards.html#assert-liberally

cfang updated this revision to Diff 410870.Feb 23 2022, 11:11 AM

Add assert for implicitarg_ptr whenever hostcall_ptr or heap_ptr is needed.

sameerds accepted this revision.Feb 25 2022, 3:24 AM
sameerds added inline comments.
llvm/lib/BinaryFormat/AMDGPUMetadataVerifier.cpp
109

Seems to be a whitespace change introduced by clang-format?

This revision is now accepted and ready to land.Feb 25 2022, 3:24 AM
cfang added inline comments.Feb 25 2022, 10:14 AM
llvm/lib/BinaryFormat/AMDGPUMetadataVerifier.cpp
109

Right, a clang-format change.

llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
413

Could not

This revision was landed with ongoing or failed builds.Feb 25 2022, 10:47 AM
This revision was automatically updated to reflect the committed changes.