[PATCH] D143498: Reapply "[AMDGPU] Modify adjustInliningThreshold to also consider the cost of passing function arguments through the stack"

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:2488-2489
     // Everything else is in VGPRs.
-    return F->getAttributes().hasParamAttr(A->getArgNo(), Attribute::InReg) ||
-           F->getAttributes().hasParamAttr(A->getArgNo(), Attribute::ByVal);
+    return Attrs.hasParamAttr(ArgNo, Attribute::InReg) ||
+           Attrs.hasParamAttr(ArgNo, Attribute::ByVal);
   default:
----------------
JanekvO wrote:
> arsenm wrote:
> > JanekvO wrote:
> > > arsenm wrote:
> > > > JanekvO wrote:
> > > > > arsenm wrote:
> > > > > > You should go through CallBase::paramHasAttr rather than directly inspecting the AttributeList 
> > > > > I tried to retain existing API for `isArgPassedInSGPR(const Argument *A)` which doesn't have access to any `CallBase`. I think I could force something with `function_ref` but not sure if that's the way to go.
> > > > Do we really need to retain the Argument version?
> > > Looking into it, there seems to be a only 2 uses of `isArgPassedInSGPR(const Argument *A)`, however, I couldn't find a way to rewrite them to use a more `CallBase` friendly API.
> > I'm not really sure what the policy is for mismatched call site and declaration ABI attributes policy is supposed to be. We don't have a verifier check for it. My current assumption is for inreg, it would be present on the declaration and never in the call site attributes which won't work here. If you were to only pick one, you would be better off going off the callee's attribute set.
> > 
> > The Argument and call case are also different. The Argument version is for incoming and CallBase for outgoing. You might just need to split the two 
> > My current assumption is for inreg, it would be present on the declaration and never in the call site attributes which won't work here. If you were to only pick one, you would be better off going off the callee's attribute set.
> 
> This might be tough in the case of indirect calls since the `CallBase` is all that's provided to the TTI hook and `CallBase::getCalledfunction` will return `nullptr`. Perhaps adding the `Callee` function to the existing TTI or a new TTI hook to adjust cost/threshold where the `Callee` function can be accessed might be more appropriate?
I think just two duplicate functions for the incoming and outgoing case is fine. One will use Argument and the other CallBase and parameter index 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143498