[PATCH] D99347: [AMDGPU] Set implicit arg attributes for indirect calls

[Jon Chesterfield via Phabricator via llvm-commits]

JonChesterfield added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:275
+
+  // If this function hasAddressTaken() = true
+  // then add all attributes corresponding to the implicit args.
----------------
Is the reasoning here that an indirect call site to an unknown callee will have set up all the implicit state, so for consistency a function which is ever called indirectly must always have all the state set up?

I would have hoped the granularity is such that functions can be annotated with only what they need, so that indirect call sites pass (and require) everything but direct call sites don't need to. With consistent register location for implicit values, I would hope that will work out of the box.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:298
+          if (!CB->isInlineAsm()) {
+            HasIndirectCall = true;
             HaveCall = true;
----------------
As discussed, presence of inline asm does not need to be taken to imply presence of an indirect call within said asm. If someone wants to make an indirect branch from asm, it's on them to get it right. If someone wants to use asm without indirect branches, they won't want the extra overhead from conservatively setting up all the state.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:382
+  //    essential for correctness).
+  if (CallingConvSupportsAllImplicits && HasIndirectCall) {
+    for (StringRef AttrName : ImplicitAttrNames) {
----------------
This doesn't make sense to me. 

An indirect call site implies all implicit args are needed, so the caller of that function will acquire those attributes. Then a direct call to said function will have those attributes, and so on.

hasAddressTaken on the kernel itself makes no difference here - is the comment out of date?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99347