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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRALongBranchReg.cpp:45
+    // of this basic block.
+    unsigned Offset = 0;
+    // Size - Size of the basic block in bytes
----------------
uint64_ts?


================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRALongBranchReg.cpp:77-78
+void GCNPreRALongBranchReg::generateBlockInfo() {
+  BlockInfo.clear();
+  BlockInfo.resize(MF->getNumBlockIDs());
+
----------------
Clear at exit instead?


================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRALongBranchReg.cpp:85
+    for (const MachineInstr &MI : MBB)
+      NumInstr += 1;
+    BlockInfo[MBB.getNumber()].Size = 8 * NumInstr;
----------------
Should skip debug and meta instructions, otherwise this violates the cardinal rule that debug info shouldn't change codegen


================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRALongBranchReg.cpp:120
+
+  for (MachineBasicBlock &MBB : *MF) {
+    MachineBasicBlock::iterator Last = MBB.getLastNonDebugInstr();
----------------
const


================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRALongBranchReg.cpp:134-136
+      Register Reg =
+          AMDGPU::SGPR_64RegClass.getRegister(STM.getMaxNumSGPRs(*MF) / 2 - 1);
+      MFI->setLongBranchReservedReg(Reg);
----------------
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


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:1429
   SIMachineFunctionInfo *FuncInfo = MF.getInfo<SIMachineFunctionInfo>();
+  bool RegsFrozen = false;
 
----------------
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


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:2555
 
   // FIXME: Virtual register workaround for RegScavenger not working with empty
   // blocks.
----------------
We should also fix this fixme at some point


================
Comment at: llvm/test/CodeGen/MIR/AMDGPU/machine-function-info-long-branch-reg.ll:66
+}
+
+attributes #0 = { nounwind }
----------------
Add a test to show no change for debug instructions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149775