[PATCH] D114652: [AMDGPU] Treat call clobbered return address registers s[30:31] as callee saved

Matt Arsenault via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Dec 1 05:57:54 PST 2021


arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp:232
+    Register Reg = TRI->getReturnAddressReg(MF);
+    if (!FuncInfo->isEntryFunction() && MFI.hasCalls()) {
+      const TargetRegisterClass *RC =
----------------
RamNalamothu wrote:
> sebastian-ne wrote:
> > Why is the return address only marked as callee-save if this function has calls? I assume it can’t it be overwritten if there are no calls because it is marked as reserved?
> > 
> > Coming back here after reading the tests, I think s[30:31] should also be marked as callee-save if there are no calls. It shouldn’t cause harm if there are no calls (the register is reserved anyway) and it should fix the tests.
> > Why is the return address only marked as callee-save if this function has calls? I assume it can’t it be overwritten if there are no calls because it is marked as reserved?
> > 
> 
> Yes.
> 
> > Coming back here after reading the tests, I think s[30:31] should also be marked as callee-save if there are no calls. It shouldn’t cause harm if there are no calls (the register is reserved anyway) and it should fix the tests.
> 
> AFAIK that's the eventual goal and there is a discussion to pick the ABI registers for return address from the callee saved range. And these changes should work with no/minimal changes.
> 
> However, I don't think I understand how the tests will get fixed automatically if the return address registers are marked as callee saved because even the SGPR CSR save/restore happens during SGPRSpillLowering which is where I am manually adding s[30:31] to the CSR list.
I don't think this register should be reserved, which avoids the need to do this


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:630-633
+  // The return address register is call clobbered and the CFI needs to track
+  // its location. Hence it is handled specially.
+  if (!MFI->isEntryFunction())
+    reserveRegisterTuples(Reserved, getReturnAddressReg(MF));
----------------
I don't think it's necessary to reserve this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114652



More information about the llvm-commits mailing list