[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 =
----------------
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


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp:1222
+      Instruction::Load, Type::getInt32Ty(Callee->getContext()),
+      TTIImpl->getDataLayout().getPointerPrefAlignment(
+          AMDGPUAS::PRIVATE_ADDRESS),
----------------
assign DL to variable above?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp:1226-1228
+  // The spill penalty is, currently, equal for either SGPR and VGPR; however,
+  // VGPR occupies more stack space in comparison which is not reflected in the
+  // penalty cost.
----------------
This isn't actually true today, we don't implement SGPR argument packing


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp:1234
+      std::max(0, VGPRsInUse - NrOfVGPRUntilSpill) *
+      *ArgSpillCost.getValue() * InlineConstants::getInstrCost();
+  return adjustThreshold;
----------------
Don't refer to this as a spill, it's a stack cost


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140242