[AMDGPU] Emit .actual_access metadata

[cfang]

Summary

Emit .actual_access metadata for the deduced argument access qualifier,
and .access for kernel_arg_access_qual.

[arsenm]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg-v3.ll

26 ↗

(On Diff #548385)

This looks like lost test coverage, need to test the access of the metadata

[cfang]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg-v3.ll

26 ↗

(On Diff #548385)

Under V3, if the access is default, it won't print out "default", (will not print out the access qualifier at all).

But I am curious because "noalias readonly %in", why the access is not "readonly"? Does it mean the original access qualifier got lost?

[arsenm]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg-v3.ll

26 ↗

(On Diff #548385)

the point of this is introspection of the source program, not the actual properties. if someone defines something as readwrite but is only ever read, the optimizer can conclude it's readonly even though it wasn't declared that way

[cfang]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg-v3.ll

26 ↗

(On Diff #548385)

Under V3 or above, it is designed NOT to print out the access qualifier if it is "default".
Do you want to CHECK-NOT for :access" for the default?

[arsenm]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg-v3.ll

2 ↗

(On Diff #548385)

From the test name, was this supposed to be inferred? Are there non-introspection uses for the access metadata?

26 ↗

(On Diff #548385)

-NEXT checks are safer than -NOT

[cfang]

Update the LIT tests with kernel_arg_access_qual specified to "read_only: for the first argument.

[cfang]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg-v3.ll

2 ↗

(On Diff #548385)

I am not sure about the actual uses. But apparently the conformance test get_kernel_arg_info checks that.
Do you want to change the name (remove deduce? ) of these two tests?

[arsenm]

The first iteration of this test in D32091 suggests this actually does something, so it seems unfortunate if we're limited by introspection tests

[rampitec]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg-v3.ll

26 ↗

(On Diff #548385)

Right, this can be deduced and had quite positive performance impact on some kernels.

[cfang]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg-v3.ll

26 ↗

(On Diff #548385)

It seems we are using the same AccQual field for two purposes: 1) optimization, 2) get_kernel_arg_info.AccessQualifier, and
the second one is expected from getMetadata("kernel_arg_access_qual")

[rampitec]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg-v3.ll

26 ↗

(On Diff #548385)

This is the same thing, one is because developer marked it (and language allowed this marking), another because we were able to prove it from compiler. Net impact is still the same, RT may omit copying memory before every dispatch.

[cfang]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg-v3.ll

26 ↗

(On Diff #548385)

We are seeing a conformance test failure with test_api get_kernel_arg_info.
The test expects access qualifier to be access_none for any kernels other than image and pipes.
Do you think the checks from this test are not reasonable?

[cfang]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg-v3.ll

26 ↗

(On Diff #548385)

I meant if the language allows us to change, it should not check for the original ones.

[rampitec]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg-v3.ll

26 ↗

(On Diff #548385)

Sigh. I do not see why the same qualifier cannot apply to other types of memory. Readonly is readonly, and the optimization is useful. I understand that the standard did not say it is allowed, but conformance did pass for a long time this optimization is there.

[yaxunl]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg-v3.ll

26 ↗

(On Diff #548385)

There is a kernel arg metadata “.actual_access” which may be used instead:

“.actual_access” string
The actual memory accesses performed by the kernel on the kernel argument. Only present if “.value_kind” is “global_buffer”, “image”, or “pipe”. This may be more restrictive than indicated by “.access” to reflect what the kernel actual does. If not present then the runtime must assume what is implied by “.access” and “.is_const” . Values are:

“read_only”
“write_only”
“read_write”

https://llvm.org/docs/AMDGPUUsage.html#code-object-v3-metadata

[rampitec]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg-v3.ll

26 ↗

(On Diff #548385)

Makes senses as long as agreed with RT. However, I would argue this shall be set in the same patch.

[cfang]

implement .actual_access metadata emission.

[arsenm]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg-v3.ll

28 ↗

(On Diff #548385)

Don't see new actual_access?

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg.ll

2–3

Seems like the mirror write only deduction test is missing

[cfang]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg.ll

2–3

We currently only emit read_only for .actual_access. Do you know what are the exact
conditions to emit "write_only" or "read_write"? Thanks.

if (Arg.getType()->isPointerTy() && Arg.onlyReadsMemory() &&
    Arg.hasNoAliasAttr()) {
  ActAccQual = "read_only";
}

[cfang]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg-v3.ll

28 ↗

(On Diff #548385)

This is the test for code object version 2. Do you think I should still update for V2?

[arsenm]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg.ll

2–3

Arg.onlyReadsMemory and Arg.hasAttribute(Attribute::WriteOnly) (don't know if you really need the noalias part here, it's probably a runtime workaround if you do)

[cfang]

add the case of deduced "write_only" emission.

[cfang]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg.ll

2–3

@rampitec: Do you remember exactly why noalias is needed?

[rampitec]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg.ll

2–3

The fact that you do not write through this pointer and pointers derived from it does not tell that you do not write this memory. You can tell you don't if there can be no other pointers to that memory as well.

[rampitec]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg.ll

2–3

Like:

void foo(const int *p1, int *p2) {
  *p2 = p1[1];
}

This is wrong to assume that memory under p1 was not modified because you can call it like this:

foo(p, p);

[arsenm]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg.ll

28

also should test without the noaliases

[cfang]

Add tests without noalias attribute.

Also plan to delete the following test in commit because all cases have been included in the added file.
llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg.ll

[arsenm]

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduced-arg-attr.ll

12 ↗

(On Diff #550122)

Reduce this down to the read_only, best to have -NOT checks as loose as possible if you aren't going to CHECK-NEXT everything

26 ↗

(On Diff #550122)

Ditto

llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.h

   void emitKernelArg(const Argument &Arg, unsigned &Offset,
                      msgpack::ArrayDocNode Args);
 
   void emitKernelArg(const DataLayout &DL, Type *Ty, Align Alignment,
                      StringRef ValueKind, unsigned &Offset,
                      msgpack::ArrayDocNode Args,
                      MaybeAlign PointeeAlign = std::nullopt,
                      StringRef Name = "", StringRef TypeName = "",
-                     StringRef BaseTypeName = "", StringRef AccQual = "",
-                     StringRef TypeQual = "");
+                     StringRef BaseTypeName = "", StringRef ActAccQual = "",
+                     StringRef AccQual = "", StringRef TypeQual = "");
 
   void emitHiddenKernelArgs(const MachineFunction &MF, unsigned &Offset,
                             msgpack::ArrayDocNode Args) override;
 
   msgpack::DocNode &getRootMetadata(StringRef Key) {
     return HSAMetadataDoc->getRoot().getMap(/*Convert=*/true)[Key];
   }
 

llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp

   if (Node && ArgNo < Node->getNumOperands())
     TypeName = cast<MDString>(Node->getOperand(ArgNo))->getString();
 
   StringRef BaseTypeName;
   Node = Func->getMetadata("kernel_arg_base_type");
   if (Node && ArgNo < Node->getNumOperands())
     BaseTypeName = cast<MDString>(Node->getOperand(ArgNo))->getString();
 
-  StringRef AccQual;
-  if (Arg.getType()->isPointerTy() && Arg.onlyReadsMemory() &&
-      Arg.hasNoAliasAttr()) {
-    AccQual = "read_only";
-  } else {
-    Node = Func->getMetadata("kernel_arg_access_qual");
-    if (Node && ArgNo < Node->getNumOperands())
-      AccQual = cast<MDString>(Node->getOperand(ArgNo))->getString();
+  StringRef ActAccQual;
+  // Do we really need NoAlias check here?
+  if (Arg.getType()->isPointerTy() && Arg.hasNoAliasAttr()) {
+    if (Arg.onlyReadsMemory())
+      ActAccQual = "read_only";
+    else if (Arg.hasAttribute(Attribute::WriteOnly))
+      ActAccQual = "write_only";
   }
 
+  StringRef AccQual;
+  Node = Func->getMetadata("kernel_arg_access_qual");
+  if (Node && ArgNo < Node->getNumOperands())
+    AccQual = cast<MDString>(Node->getOperand(ArgNo))->getString();
+
   StringRef TypeQual;
   Node = Func->getMetadata("kernel_arg_type_qual");
   if (Node && ArgNo < Node->getNumOperands())
     TypeQual = cast<MDString>(Node->getOperand(ArgNo))->getString();
 
   const DataLayout &DL = Func->getParent()->getDataLayout();
 
   MaybeAlign PointeeAlign;
   Type *Ty = Arg.hasByRefAttr() ? Arg.getParamByRefType() : Arg.getType();
 
   // FIXME: Need to distinguish in memory alignment from pointer alignment.
   if (auto PtrTy = dyn_cast<PointerType>(Ty)) {
     if (PtrTy->getAddressSpace() == AMDGPUAS::LOCAL_ADDRESS)
       PointeeAlign = Arg.getParamAlign().valueOrOne();
   }
 
   // There's no distinction between byval aggregates and raw aggregates.
   Type *ArgTy;
   Align ArgAlign;
   std::tie(ArgTy, ArgAlign) = getArgumentTypeAlign(Arg, DL);
 
   emitKernelArg(DL, ArgTy, ArgAlign,
                 getValueKind(ArgTy, TypeQual, BaseTypeName), Offset, Args,
-                PointeeAlign, Name, TypeName, BaseTypeName, AccQual, TypeQual);
+                PointeeAlign, Name, TypeName, BaseTypeName, ActAccQual,
+                AccQual, TypeQual);
 }
 
 void MetadataStreamerMsgPackV3::emitKernelArg(
     const DataLayout &DL, Type *Ty, Align Alignment, StringRef ValueKind,
     unsigned &Offset, msgpack::ArrayDocNode Args, MaybeAlign PointeeAlign,
     StringRef Name, StringRef TypeName, StringRef BaseTypeName,
-    StringRef AccQual, StringRef TypeQual) {
+    StringRef ActAccQual, StringRef AccQual, StringRef TypeQual) {
   auto Arg = Args.getDocument()->getMapNode();
 
   if (!Name.empty())
     Arg[".name"] = Arg.getDocument()->getNode(Name, /*Copy=*/true);
   if (!TypeName.empty())
     Arg[".type_name"] = Arg.getDocument()->getNode(TypeName, /*Copy=*/true);
   auto Size = DL.getTypeAllocSize(Ty);
   Arg[".size"] = Arg.getDocument()->getNode(Size);

       // Limiting address space to emit only for a certain ValueKind.
       if (ValueKind == "global_buffer" || ValueKind == "dynamic_shared_pointer")
         Arg[".address_space"] = Arg.getDocument()->getNode(*Qualifier,
                                                            /*Copy=*/true);
 
   if (auto AQ = getAccessQualifier(AccQual))
     Arg[".access"] = Arg.getDocument()->getNode(*AQ, /*Copy=*/true);
 
-  // TODO: Emit Arg[".actual_access"].
+  if (auto AAQ = getAccessQualifier(ActAccQual))
+    Arg[".actual_access"] = Arg.getDocument()->getNode(*AAQ, /*Copy=*/true);
 
   SmallVector<StringRef, 1> SplitTypeQuals;
   TypeQual.split(SplitTypeQuals, " ", -1, false);
   for (StringRef Key : SplitTypeQuals) {
     if (Key == "const")
       Arg[".is_const"] = Arg.getDocument()->getNode(true);
     else if (Key == "restrict")
       Arg[".is_restrict"] = Arg.getDocument()->getNode(true);

llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg.ll

 ; RUN: llc -mtriple=amdgcn-amd-amdhsa -filetype=obj -o - < %s | llvm-readelf --notes - | FileCheck %s
 
 ; CHECK:        - .args:
-; CHECK-NEXT:       - .access:         read_only
+; CHECK-NEXT:       - .actual_access:  read_only
 ; CHECK-NEXT:         .address_space:  global
 ; CHECK-NEXT:         .is_const:       true
 ; CHECK-NEXT:         .is_restrict:    true
 ; CHECK-NEXT:         .name:           in
 ; CHECK-NEXT:         .offset:         0
 ; CHECK-NEXT:         .size:           8
 ; CHECK-NEXT:         .type_name:      'float*'
 ; CHECK-NEXT:         .value_kind:     global_buffer
-; CHECK-NEXT:       - .address_space:  global
+; CHECK-NEXT:       - .actual_access:  write_only
+; CHECK-NEXT:         .address_space:  global
 ; CHECK-NEXT:         .name:           out
 ; CHECK-NEXT:         .offset:         8
 ; CHECK-NEXT:         .size:           8
 ; CHECK-NEXT:         .type_name:      'float*'
 ; CHECK-NEXT:         .value_kind:     global_buffer
 ; CHECK:          .name:           test_ro_arg
 ; CHECK:          .symbol:         test_ro_arg.kd
 
-define amdgpu_kernel void @test_ro_arg(ptr addrspace(1) noalias readonly %in, ptr addrspace(1) %out)
+define amdgpu_kernel void @test_ro_arg(ptr addrspace(1) noalias readonly %in, ptr addrspace(1) noalias writeonly %out)
     !kernel_arg_addr_space !0 !kernel_arg_access_qual !1 !kernel_arg_type !2
     !kernel_arg_base_type !2 !kernel_arg_type_qual !3 {
   ret void
 }
 
 !0 = !{i32 1, i32 1}
 !1 = !{!"none", !"none"}
 !2 = !{!"float*", !"float*"}
 !3 = !{!"const restrict", !""}