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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm requested changes to this revision.
arsenm added a comment.
This revision now requires changes to proceed.

I don't want any usage of metadata, it's confusing and unnecessary. The layout should be fully deterministic from just the argument list and inreg, you don't need a parallel encoding scheme



================
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(
----------------
kerbowa wrote:
> 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.
This is an anti feature. Just respect inreg regardless of the source. You're just adding a huge source of fragility just in case a frontend tries to make use of an in progress feature


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156852