[PATCH] D123525: [AMDGPU] On gfx908, reserve VGPR for AGPR copy based on register budget.

Mon Apr 11 19:09:22 PDT 2022

hsmhsm added inline comments.

================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:628
   // or spill with the slot.
   while (RegNo-- && RS.FindUnusedReg(&AMDGPU::VGPR_32RegClass)) {
     Register Tmp2 = RS.scavengeRegister(&AMDGPU::VGPR_32RegClass, 0);
----------------
rampitec wrote:
> It will never get replaced if RegNo == 0.
Yes, I noticed it. I think, we need to take a relook at it.

================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h:520
+    VGPRForAGPRCopy =
+        (HighestAvailableVGPR < 16)
+            ? AMDGPU::VGPR_32RegClass.getRegister(32)
----------------
rampitec wrote:
> I do not know real world situation with such low budget.
Even, I had my own doubt about such a low budget before submitting the patch, But, I thought - let me post a patch, discus it open, and revise it based on the outcome of the discussion.

================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h:521
+        (HighestAvailableVGPR < 16)
+            ? AMDGPU::VGPR_32RegClass.getRegister(32)
+            : AMDGPU::VGPR_32RegClass.getRegister(HighestAvailableVGPR);
----------------
rampitec wrote:
> And then 32 is over the budget. This is a bug.
Really, I have no idea what to do in case of constrained register budget - Stealing one more register from already constrained set would make RegAlloc to fail because of "no register".

However, I think, we need to discuss it, and refine it based on common consensus.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123525