[PATCH] D119027: [AMDGPU][NFC]: Emit metadata for hidden_heap_v1 kernarg

[Changpeng Fang via Phabricator via llvm-commits]

cfang added inline comments.


================
Comment at: llvm/lib/BinaryFormat/AMDGPUMetadataVerifier.cpp:109
     return false;
-  if (!verifyScalarEntry(ArgsMap, ".value_kind", true,
-                         msgpack::Type::String,
+  if (!verifyScalarEntry(ArgsMap, ".value_kind", true, msgpack::Type::String,
                          [](msgpack::DocNode &SNode) {
----------------
sameerds wrote:
> Seems to be a whitespace change introduced by clang-format?
Right, a clang-format change.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:413
-    if (funcRetrievesHostcallPtr(A)) {
-      removeAssumedBits(IMPLICIT_ARG_PTR);
       removeAssumedBits(HOSTCALL_PTR);
----------------
sameerds wrote:
> cfang wrote:
> > sameerds wrote:
> > > cfang wrote:
> > > > sameerds wrote:
> > > > > Why was this removed? Is there some other place which guarantees the same relationship?
> > > > I think the explicit call of amdgcn_implicitarg_ptr will guarantee IMPLICIT_ARG_PTR.
> > > That sounds correct. But then this should be asserted somewhere ... probably in the code that manifests the attribute.
> > 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()) {
> 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
Could not


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

https://reviews.llvm.org/D119027