[PATCH] D70379: [AMDGPU] Reserving VGPR for future SGPR Spill

[Saiyedul Islam via Phabricator via llvm-commits]

saiislam marked 2 inline comments as done.
saiislam added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:100
+    "amdgpu-reserve-vgpr-for-sgpr-spill",
+    cl::desc("Allocates one VGPR for future SGPR Spill"), cl::init(false));
+
----------------
arsenm wrote:
> This should be the default, but the test disruption is probably significant without forcing the VGPR to be the high register, and adjust it down after RA
Yes, you are right. 285 test cases in AMDGPU codegen are failing by making it default, in the current state.


================
Comment at: llvm/test/CodeGen/AMDGPU/reserve-vgpr-for-sgpr-spill.ll:12
+; GCN: ; NumVgprs: 256
+define fastcc i32 @parent_func(i32 %0, i32 %1, [255 x i32] %b) #1 {
+entry:
----------------
arsenm wrote:
> I don't see how this test really stresses the need for SGPR spilling with no free VGPRs. I would expect this to look more like the tests in spill-wide-sgpr.ll, or spill-scavenge-offset.ll to force a high pressure
The arguments of the function are taking up all the 256 VGPRs, so FP (s34) and SGPRs (s30 and s31) are getting spilled into reserved VGPR (v32), and restored later.
Tests in spill-wide-sgpr.ll are also enforcing 2,4,8 way spilling of SGPR, which I can do here by modifying the body of parent_func while keeping the function signature of both functions. Will it be ok then?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70379