[PATCH] D149775: [AMDGPU] Reserve SGPR pair when long branches are present
Matt Arsenault via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 21 07:00:04 PDT 2023
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
More information about the llvm-commits
mailing list