[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:116-117
+  constexpr bool ReserveHighestRegister = true;
+  Register LongBranchReservedReg = TRI->findUnusedRegister(
+      MRI, &AMDGPU::SGPR_64RegClass, MF, ReserveHighestRegister);
+  if (!LongBranchReservedReg) {
----------------
I don't see how this can fail unless you're using amdgpu-num-sgprs (and this is one of the reasons it should be removed)


================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRALongBranchReg.cpp:119
+  if (!LongBranchReservedReg) {
+    MFI->setLongBranchReservedReg(AMDGPU::NoRegister);
+    return false;
----------------
I would assume this is already noregister if this failed

I think you misunderstood what I was saying about reserve first and release later. I meant in the compile pipeline, not in the pass itself. You can find the reserved register in finalizeLowering, and then have this pass unset and un-reserve. I don't know if we have an exposed helper to clear reserved registers. You can do that as a follow up,


================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRALongBranchReg.cpp:120
+    MFI->setLongBranchReservedReg(AMDGPU::NoRegister);
+    return false;
+  }
----------------
If you actually did modify the MFI register this should be a return true


================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRALongBranchReg.cpp:143
+  MFI->setLongBranchReservedReg(AMDGPU::NoRegister);
+  return false;
+}
----------------
This should be return true if you modified MFI


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:2624
+  // avoid running the scavenger and just use those registers
+  if (LongBranchReservedReg)
+    Scav = LongBranchReservedReg;
----------------
Should also skip the enterBasicBlockEnd above


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149775