Emit metadata for hidden_heap_v1 kernarg
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp | ||
---|---|---|
521 | We could potentially be more specific with KernargLoc instead of MemoryLoc. |
llvm/lib/Target/AMDGPU/AMDGPUAttributes.def | ||
---|---|---|
22 | It seems we need to document these attributes in AMDGPUUsage, which I missed for no-hostcall-ptr. | |
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. |
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. |
Add descriptions of amdgpu-no-hostcall-ptr and amdgpu-no-heap-ptr in the docs.
Update a couple of function names.
llvm/lib/Target/AMDGPU/AMDGPUAttributes.def | ||
---|---|---|
22 | Agreed! Thanks for the suggestions. |
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. |
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. |
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()) { |
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 |
llvm/lib/BinaryFormat/AMDGPUMetadataVerifier.cpp | ||
---|---|---|
109 | Seems to be a whitespace change introduced by clang-format? |
Seems to be a whitespace change introduced by clang-format?