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

[Sebastian Neubauer via Phabricator via llvm-commits]

sebastian-ne added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp:94-95
       MachineInstrSpan MIS(I, &SaveBlock);
-      const TargetRegisterClass *RC =
-        TRI->getMinimalPhysRegClass(Reg, MVT::i32);
+      const TargetRegisterClass *RC = TRI->getMinimalPhysRegClass(
+          Reg, Reg == RI->getReturnAddressReg(MF) ? MVT::i64 : MVT::i32);
 
----------------
Can the MVT be deduced from `getRegSizeInBits(*getPhysRegClass(Reg))`? I think that would look cleaner.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp:232
+    Register Reg = TRI->getReturnAddressReg(MF);
+    if (!FuncInfo->isEntryFunction() && MFI.hasCalls()) {
+      const TargetRegisterClass *RC =
----------------
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.


================
Comment at: llvm/test/CodeGen/AMDGPU/branch-relax-spill.ll:897
 
 define void @spill_func(i32 addrspace(1)* %arg) #0 {
 ; CHECK-LABEL: spill_func:
----------------
This test is not working with this patch. It has no calls, so s[30:31] are not marked as callee-save. But they are overwritten in inline assembly.
Before this patch, they were saved and restored. With this patch this is not the case and the return will jump to an invalid address.


================
Comment at: llvm/test/CodeGen/AMDGPU/call-preserved-registers.ll:75-84
+; GCN-LABEL: {{^}}void_func_void_clobber_s28_s29:
 ; GCN: s_waitcnt
-; GCN-NEXT: s_mov_b64 [[SAVEPC:s\[[0-9]+:[0-9]+\]]], s[30:31]
 ; GCN-NEXT: #ASMSTART
 ; GCN: ; clobber
 ; GCN-NEXT: #ASMEND
-; GCN-NEXT: s_setpc_b64 [[SAVEPC]]
-define void @void_func_void_clobber_s30_s31() #2 {
-  call void asm sideeffect "; clobber", "~{s[30:31]}"() #0
+; GCN-NEXT: s_setpc_b64 s[30:31]
+define void @void_func_void_clobber_s28_s29() #2 {
----------------
I think this test is supposed to clobber the return address in s[30:31].


================
Comment at: llvm/test/CodeGen/AMDGPU/callee-frame-setup.ll:426
     ,~{s30},~{s31}"() #0
-
+  call void @local_empty_func()
   ret void
----------------
I think these tests should continue to work without the need to introduce a call.


================
Comment at: llvm/test/CodeGen/AMDGPU/gfx-callable-preserved-registers.ll:127
+; GCN:          v_readlane_b32 s29, v0, 1
+  call void asm sideeffect "; clobber", "~{s[28:29]}"() #0
   ret void
----------------
I think this test purposefully clobbered the return address in s[30:31].


================
Comment at: llvm/test/CodeGen/AMDGPU/gfx-callable-return-types.ll:26-48
+; GFX9-NEXT:    s_or_saveexec_b64 s[34:35], -1
+; GFX9-NEXT:    buffer_store_dword v1, off, s[0:3], s32 ; 4-byte Folded Spill
+; GFX9-NEXT:    s_mov_b64 exec, s[34:35]
+; GFX9-NEXT:    v_writelane_b32 v1, s33, 2
+; GFX9-NEXT:    s_mov_b32 s33, s32
+; GFX9-NEXT:    s_addk_i32 s32, 0x400
+; GFX9-NEXT:    s_getpc_b64 s[34:35]
----------------
This codes gets quite a bit worse. But I’m not sure if we can do anything about it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114652