[PATCH] D149775: [AMDGPU] Reserve SGPR pair when long branches are present

[Corbin Robeck via Phabricator via llvm-commits]

crobeck marked an inline comment as done.
crobeck added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRALongBranchReg.cpp:134-136
+      Register Reg =
+          AMDGPU::SGPR_64RegClass.getRegister(STM.getMaxNumSGPRs(*MF) / 2 - 1);
+      MFI->setLongBranchReservedReg(Reg);
----------------
arsenm wrote:
> This should be moved into a TRI reserve registers helper like the others.
> 
> Also, would be a bit safer to invert this by reserving by default and having this pass *remove* the reserved register
We're not actually reserving the register here. We're just setting the flag that we need to reserve the register in TRI. But, I can move this if it makes more sense there.

If we instead invert it we then need to always run through everything and prove there isn't a long branch as opposed to exiting once if we find a long branch. 


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:1429
   SIMachineFunctionInfo *FuncInfo = MF.getInfo<SIMachineFunctionInfo>();
+  bool RegsFrozen = false;
 
----------------
arsenm wrote:
> The reserved registers should already be frozen before PEI. I think the current freezeReservedRegs call here was just never updated to use the new incremental reserveReg
Just set both calls to reserveReg then?


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:1445
       FuncInfo->setVGPRForAGPRCopy(UnusedLowVGPR);
       MRI.freezeReservedRegs(MF);
+      RegsFrozen = true;
----------------
cdevadas wrote:
> Unify the `freezeReservedRegs` at the end of the function?
Pending updating reserveReg call above.


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

https://reviews.llvm.org/D149775