[PATCH] D130784: [AMDGPU] Support LDS spilling

[Piotr Sobczak via Phabricator via llvm-commits]

piotr added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:2560
+  const GCNSubtarget &ST = MF.getSubtarget<GCNSubtarget>();
+  if (MFI->ldsSpillingEnabled(MF) && CallConv == CallingConv::AMDGPU_CS &&
+      ST.getFlatWorkGroupSizes(Fn).second > ST.getWavefrontSize()) {
----------------
rampitec wrote:
> Why CS only?
This is a left-over from an older version of the patch, will remove.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:2565
+      auto OrigArg = Fn.getArg(Arg.getOrigArgIndex());
+      if (OrigArg->getName().equals("MultiDispatchInfo")) {
+        CCValAssign &VA = ArgLocs[i];
----------------
rampitec wrote:
> Matching argument by name is bad, especially if such name can be used by an user.
This is controlled by the front-end - graphics compute shaders do not have inputs that could be specified by a shader writer and end up landing here.

Having said that, I am not 100% happy with the way this is handled here either. I guess a better idea could be to rely on a function attribute set by the front-end that would say - "the user sgpr you want to use for multi dispatch info is at nth location". 

Just to note, input sgprs are handled differently between graphics and compute. I need to "just" preload WorkGroupInfo, so that I could later use that sgpr in the spill code. In kernels, it is treated as a system sgpr (allocateSystemSGPRs), so preloading it should not be a problem. However, in graphics we treat it as a user sgpr and pass in the list of arguments (with the name "MultiDispatchInfo"). 




================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp:741
+  const GCNSubtarget &ST = MF.getSubtarget<GCNSubtarget>();
+  if (ST.getLdsSpillLimitDwords(MF) == 0)
+    return false;
----------------
rampitec wrote:
> This is the most expensive check, it should go last.
Ok, will re-order. (I placed this condition at the beginning, because it would be true (== return early) more often the the other checks).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130784