[PATCH] D140242: [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/AMDGPUTargetTransformInfo.cpp:1193-1194
+  for (const Argument &A : Callee->args()) {
+    MVT ArgType = MVT::getVT(A.getType());
+    if (A.getType()->isPointerTy()) {
+      ArgType =
----------------
arsenm wrote:
> JanekvO wrote:
> > arsenm wrote:
> > > JanekvO wrote:
> > > > arsenm wrote:
> > > > > Can you just go through EVT? This is going to not work for vectors of pointers. You shouldn't have to consider any of these details
> > > > > This is going to not work for vectors of pointers.
> > > > Sorry, is there a way to get vector of pointers working with EVT that I'm missing?
> > > You should be performing no logic on the type. You should pass the raw type to getEVT (i.e. remove the isPointerTy check, that doesn't cover vectors of pointers)
> > Ah, the reason I put some type logic here is because `getNumRegistersForCallingConv` calls `EVT::getSizeInBits` which is target dependent and will hit an unreachable error (`EVT::getEVT` also doesn't allow vector of pointers).
> I forgot this API was bad. You probably want getValueType (which is what TTI::getRegUsageForType uses, and also won't work correctly for aggregates). The most correct thing would be to call ComputeValueVTs to cover all aggregates correctly, but that feels a bit heavy. I'd go with getTLI()->getValueType(), and add a test with an array and a struct argument just to make sure those don't break too badly
Actually, we kind of care a lot about getting aggregates correct. We generate a number of them for the basic HIP ABI and should be emitting more of them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140242