[PATCH] D156852: [AMDGPU] Use inreg for hint to preload kernel arguments

[Austin Kerbow via Phabricator via llvm-commits]

kerbowa marked 3 inline comments as done.
kerbowa added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:933
+    // Check for incompatible attributes.
+    if (Arg.hasByRefAttr() || Arg.hasNestAttr())
+      break;
----------------
arsenm wrote:
> Could actually handle byref (and should, ideally all kernargs would use byref). If we consistently used byref, you could implement this by moving memory values to the arg list
I'm planning on adding it in a future revision but I don't think it's needed for this iteration of the changes.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:945
+    MDBuilder MDB(Ctx);
+    F.setMetadata("preload_kernel_args",
+                  MDNode::get(Ctx, MDB.createConstant(llvm::ConstantInt::get(
----------------
arsenm wrote:
> I don't understand why you need this metadata. You mark the arguments with inreg which provides the same information? You can still ignore the hint in the codegen
I'm adding the metadata early to ensure that kernarg preloading is constrained to those that are explicitly using the CL flag. In case there are some frontend that may sneak inreg on some arguments. Will probably just until the feature is more mature or is enabled by default.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156852