[AMDGPU] We would need FP if there is call and caller save VGPR spills

[RamNalamothu]

Summary

Since https://reviews.llvm.org/D98319, determineCalleeSavesSGPR() needs
to consider caller save VGPR spills as well while anticipating if we
require FP.

Fixes: SWDEV-295978

[arsenm]

LGTM except the test is using dead code. I also think we ought to be able to remove VGPRReservedForSGPRSpill now that SGPR and VGPR allocation are split

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

1310

Checking TRI->spillSGPRToVGPR() should be redundant

llvm/test/CodeGen/AMDGPU/need-fp-from-caller-save-vgpr-spill.ll

17

The cast/alloca are dead

51–52

The cast/alloca are dead

90–91

The cast/alloca are dead

[RamNalamothu]

Address review comments.

[RamNalamothu]

In D106758#2904887, @arsenm wrote:

LGTM except the test is using dead code. I also think we ought to be able to remove VGPRReservedForSGPRSpill now that SGPR and VGPR allocation are split

I think that's out of the scope for this review.

[arsenm]

In D106758#2907086, @RamNalamothu wrote:

In D106758#2904887, @arsenm wrote:

LGTM except the test is using dead code. I also think we ought to be able to remove VGPRReservedForSGPRSpill now that SGPR and VGPR allocation are split

I think that's out of the scope for this review.

Yes, but would be a good follow up change.

This test looks nearly identical to test/CodeGen/AMDGPU/need-fp-from-csr-vgpr-spill.ll. Can the two be merged? I don't actually see why the existing test didn't catch this and isn't changed

[RamNalamothu]

In D106758#2907507, @arsenm wrote:

In D106758#2907086, @RamNalamothu wrote:

In D106758#2904887, @arsenm wrote:

LGTM except the test is using dead code. I also think we ought to be able to remove VGPRReservedForSGPRSpill now that SGPR and VGPR allocation are split

I think that's out of the scope for this review.

Yes, but would be a good follow up change.

This test looks nearly identical to test/CodeGen/AMDGPU/need-fp-from-csr-vgpr-spill.ll. Can the two be merged? I don't actually see why the existing test didn't catch this and isn't changed.

Because test/CodeGen/AMDGPU/need-fp-from-csr-vgpr-spill.ll covers the path where we have only CSR VGPR spills and llvm/test/CodeGen/AMDGPU/need-fp-from-caller-save-vgpr-spill.ll covers the other scenario where we might reserve a caller save VGPR for SGPR spill.

Will merge both tests/files into test/CodeGen/AMDGPU/need-fp-from-vgpr-spills.ll.

[RamNalamothu]

Merged test/CodeGen/AMDGPU/need-fp-from-csr-vgpr-spill.ll and llvm/test/CodeGen/AMDGPU/need-fp-from-caller-save-vgpr-spill.ll into test/CodeGen/AMDGPU/need-fp-from-vgpr-spills.ll.

In D106758#2904887, @arsenm wrote:

LGTM except the test is using dead code. I also think we ought to be able to remove VGPRReservedForSGPRSpill now that SGPR and VGPR allocation are split

I think that's out of the scope for this review.

Yes, but would be a good follow up change.

OK, will try that.

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

void SIFrameLowering::determineCalleeSavesSGPR(MachineFunction &MF,
   SavedRegs.reset(MFI->getStackPtrOffsetReg());
 
   const BitVector AllSavedRegs = SavedRegs;
   SavedRegs.clearBitsInMask(TRI->getAllVectorRegMask());
 
   // If clearing VGPRs changed the mask, we will have some CSR VGPR spills.
   const bool HaveAnyCSRVGPR = SavedRegs != AllSavedRegs;
 
-  // We have to anticipate introducing CSR VGPR spills if we don't have any
-  // stack objects already, since we require an FP if there is a call and stack.
+  // We have to anticipate introducing CSR VGPR spills or spill of caller
+  // save VGPR reserved for SGPR spills as we now always create stack entry
+  // for it, if we don't have any stack objects already, since we require
+  // an FP if there is a call and stack.
   MachineFrameInfo &FrameInfo = MF.getFrameInfo();
-  const bool WillHaveFP = FrameInfo.hasCalls() && HaveAnyCSRVGPR;
+  const bool WillHaveFP = FrameInfo.hasCalls() &&
+                          (HaveAnyCSRVGPR || (TRI->spillSGPRToVGPR() &&
+                                              MFI->VGPRReservedForSGPRSpill));
 
   // FP will be specially managed like SP.
   if (WillHaveFP || hasFP(MF))
     SavedRegs.reset(MFI->getFrameOffsetReg());
 }
 
 bool SIFrameLowering::assignCalleeSavedSpillSlots(
     MachineFunction &MF, const TargetRegisterInfo *TRI,

llvm/test/CodeGen/AMDGPU/gfx-callable-preserved-registers.ll

 
 define amdgpu_gfx void @test_call_void_func_void_preserves_s33(i32 addrspace(1)* %out) #0 {
 ; GFX9-LABEL: test_call_void_func_void_preserves_s33:
 ; GFX9:       ; %bb.0:
 ; GFX9-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; GFX9-NEXT:    s_or_saveexec_b64 s[4:5], -1
 ; GFX9-NEXT:    buffer_store_dword v40, off, s[0:3], s32 ; 4-byte Folded Spill
 ; GFX9-NEXT:    s_mov_b64 exec, s[4:5]
-; GFX9-NEXT:    v_writelane_b32 v40, s33, 3
+; GFX9-NEXT:    v_writelane_b32 v40, s33, 2
+; GFX9-NEXT:    v_writelane_b32 v40, s30, 0
 ; GFX9-NEXT:    s_mov_b32 s33, s32
-; GFX9-NEXT:    v_writelane_b32 v40, s33, 0
-; GFX9-NEXT:    v_writelane_b32 v40, s30, 1
 ; GFX9-NEXT:    s_addk_i32 s32, 0x400
 ; GFX9-NEXT:    s_getpc_b64 s[4:5]
 ; GFX9-NEXT:    s_add_u32 s4, s4, external_void_func_void@rel32@lo+4
 ; GFX9-NEXT:    s_addc_u32 s5, s5, external_void_func_void@rel32@hi+12
-; GFX9-NEXT:    v_writelane_b32 v40, s31, 2
+; GFX9-NEXT:    v_writelane_b32 v40, s31, 1
 ; GFX9-NEXT:    ;;#ASMSTART
 ; GFX9-NEXT:    ; def s33
 ; GFX9-NEXT:    ;;#ASMEND
 ; GFX9-NEXT:    s_swappc_b64 s[30:31], s[4:5]
+; GFX9-NEXT:    v_readlane_b32 s4, v40, 0
 ; GFX9-NEXT:    ;;#ASMSTART
 ; GFX9-NEXT:    ; use s33
 ; GFX9-NEXT:    ;;#ASMEND
-; GFX9-NEXT:    v_readlane_b32 s4, v40, 1
-; GFX9-NEXT:    v_readlane_b32 s33, v40, 0
-; GFX9-NEXT:    v_readlane_b32 s5, v40, 2
+; GFX9-NEXT:    v_readlane_b32 s5, v40, 1
 ; GFX9-NEXT:    s_addk_i32 s32, 0xfc00
-; GFX9-NEXT:    v_readlane_b32 s33, v40, 3
+; GFX9-NEXT:    v_readlane_b32 s33, v40, 2
 ; GFX9-NEXT:    s_or_saveexec_b64 s[6:7], -1
 ; GFX9-NEXT:    buffer_load_dword v40, off, s[0:3], s32 ; 4-byte Folded Reload
 ; GFX9-NEXT:    s_mov_b64 exec, s[6:7]
 ; GFX9-NEXT:    s_waitcnt vmcnt(0)
 ; GFX9-NEXT:    s_setpc_b64 s[4:5]
 ;
 ; GFX10-LABEL: test_call_void_func_void_preserves_s33:
 ; GFX10:       ; %bb.0:
 ; GFX10-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; GFX10-NEXT:    s_waitcnt_vscnt null, 0x0
 ; GFX10-NEXT:    s_or_saveexec_b32 s4, -1
 ; GFX10-NEXT:    buffer_store_dword v40, off, s[0:3], s32 ; 4-byte Folded Spill
 ; GFX10-NEXT:    s_waitcnt_depctr 0xffe3
 ; GFX10-NEXT:    s_mov_b32 exec_lo, s4
-; GFX10-NEXT:    v_writelane_b32 v40, s33, 3
+; GFX10-NEXT:    v_writelane_b32 v40, s33, 2
 ; GFX10-NEXT:    s_mov_b32 s33, s32
 ; GFX10-NEXT:    s_addk_i32 s32, 0x200
 ; GFX10-NEXT:    s_getpc_b64 s[4:5]
 ; GFX10-NEXT:    s_add_u32 s4, s4, external_void_func_void@rel32@lo+4
 ; GFX10-NEXT:    s_addc_u32 s5, s5, external_void_func_void@rel32@hi+12
-; GFX10-NEXT:    v_writelane_b32 v40, s33, 0
 ; GFX10-NEXT:    ;;#ASMSTART
 ; GFX10-NEXT:    ; def s33
 ; GFX10-NEXT:    ;;#ASMEND
-; GFX10-NEXT:    v_writelane_b32 v40, s30, 1
-; GFX10-NEXT:    v_writelane_b32 v40, s31, 2
+; GFX10-NEXT:    v_writelane_b32 v40, s30, 0
+; GFX10-NEXT:    v_writelane_b32 v40, s31, 1
 ; GFX10-NEXT:    s_swappc_b64 s[30:31], s[4:5]
+; GFX10-NEXT:    v_readlane_b32 s4, v40, 0
 ; GFX10-NEXT:    ;;#ASMSTART
 ; GFX10-NEXT:    ; use s33
 ; GFX10-NEXT:    ;;#ASMEND
-; GFX10-NEXT:    v_readlane_b32 s4, v40, 1
-; GFX10-NEXT:    v_readlane_b32 s33, v40, 0
-; GFX10-NEXT:    v_readlane_b32 s5, v40, 2
+; GFX10-NEXT:    v_readlane_b32 s5, v40, 1
 ; GFX10-NEXT:    s_addk_i32 s32, 0xfe00
-; GFX10-NEXT:    v_readlane_b32 s33, v40, 3
+; GFX10-NEXT:    v_readlane_b32 s33, v40, 2
 ; GFX10-NEXT:    s_or_saveexec_b32 s6, -1
 ; GFX10-NEXT:    buffer_load_dword v40, off, s[0:3], s32 ; 4-byte Folded Reload
 ; GFX10-NEXT:    s_waitcnt_depctr 0xffe3
 ; GFX10-NEXT:    s_mov_b32 exec_lo, s6
 ; GFX10-NEXT:    s_waitcnt vmcnt(0)
 ; GFX10-NEXT:    s_setpc_b64 s[4:5]
   %s33 = call i32 asm sideeffect "; def $0", "={s33}"()
   call amdgpu_gfx void @external_void_func_void()

llvm/test/CodeGen/AMDGPU/need-fp-from-caller-save-vgpr-spill.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx803 -verify-machineinstrs -O0 < %s | FileCheck %s
+
+define hidden i32 @baz() #1 {
+; CHECK-LABEL: baz:
+; CHECK:       ; %bb.0: ; %entry
+; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; CHECK-NEXT:    s_mov_b32 s4, s33
+; CHECK-NEXT:    s_mov_b32 s33, s32
+; CHECK-NEXT:    s_add_i32 s32, s32, 0x200
+; CHECK-NEXT:    v_mov_b32_e32 v0, 0
+; CHECK-NEXT:    s_add_i32 s32, s32, 0xfffffe00
+; CHECK-NEXT:    s_mov_b32 s33, s4
+; CHECK-NEXT:    s_setpc_b64 s[30:31]
+entry:
+  %retval = alloca i32, align 4, addrspace(5)
+  %retval.ascast = addrspacecast i32 addrspace(5)* %retval to i32*
+  ret i32 0
+}
+
+define hidden i32 @bar() #0 {
+; CHECK-LABEL: bar:
+; CHECK:       ; %bb.0: ; %entry
+; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; CHECK-NEXT:    s_or_saveexec_b64 s[6:7], -1
+; CHECK-NEXT:    buffer_store_dword v1, off, s[0:3], s32 offset:4 ; 4-byte Folded Spill
+; CHECK-NEXT:    s_mov_b64 exec, s[6:7]
+; CHECK-NEXT:    v_writelane_b32 v1, s33, 2
+; CHECK-NEXT:    s_mov_b32 s33, s32
+; CHECK-NEXT:    s_add_i32 s32, s32, 0x400
+; CHECK-NEXT:    v_writelane_b32 v1, s30, 0
+; CHECK-NEXT:    v_writelane_b32 v1, s31, 1
+; CHECK-NEXT:    s_getpc_b64 s[6:7]
+; CHECK-NEXT:    s_add_u32 s6, s6, baz@rel32@lo+4
+; CHECK-NEXT:    s_addc_u32 s7, s7, baz@rel32@hi+12
+; CHECK-NEXT:    s_mov_b64 s[10:11], s[2:3]
+; CHECK-NEXT:    s_mov_b64 s[8:9], s[0:1]
+; CHECK-NEXT:    s_mov_b64 s[0:1], s[8:9]
+; CHECK-NEXT:    s_mov_b64 s[2:3], s[10:11]
+; CHECK-NEXT:    s_swappc_b64 s[30:31], s[6:7]
+; CHECK-NEXT:    v_readlane_b32 s30, v1, 0
+; CHECK-NEXT:    v_readlane_b32 s31, v1, 1
+; CHECK-NEXT:    s_add_i32 s32, s32, 0xfffffc00
+; CHECK-NEXT:    v_readlane_b32 s33, v1, 2
+; CHECK-NEXT:    s_or_saveexec_b64 s[4:5], -1
+; CHECK-NEXT:    buffer_load_dword v1, off, s[0:3], s32 offset:4 ; 4-byte Folded Reload
+; CHECK-NEXT:    s_mov_b64 exec, s[4:5]
+; CHECK-NEXT:    s_waitcnt vmcnt(0)
+; CHECK-NEXT:    s_setpc_b64 s[30:31]
+entry:
+  %retval = alloca i32, align 4, addrspace(5)
+  %retval.ascast = addrspacecast i32 addrspace(5)* %retval to i32*
+  %call = call i32 @baz()
+  ret i32 %call
+}
+
+; Has no stack objects, but introduces them due to the stack object
+; created always for the caller save VGPR spill reserved for the SGPR
+; spills. We should not have redundant spills of s33.
+define hidden i32 @foo() #0 {
+; CHECK-LABEL: foo:
+; CHECK:       ; %bb.0: ; %entry
+; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; CHECK-NEXT:    s_or_saveexec_b64 s[6:7], -1
+; CHECK-NEXT:    buffer_store_dword v2, off, s[0:3], s32 offset:4 ; 4-byte Folded Spill
+; CHECK-NEXT:    s_mov_b64 exec, s[6:7]
+; CHECK-NEXT:    v_writelane_b32 v2, s33, 2
+; CHECK-NEXT:    s_mov_b32 s33, s32
+; CHECK-NEXT:    s_add_i32 s32, s32, 0x400
+; CHECK-NEXT:    v_writelane_b32 v2, s30, 0
+; CHECK-NEXT:    v_writelane_b32 v2, s31, 1
+; CHECK-NEXT:    s_getpc_b64 s[6:7]
+; CHECK-NEXT:    s_add_u32 s6, s6, bar@rel32@lo+4
+; CHECK-NEXT:    s_addc_u32 s7, s7, bar@rel32@hi+12
+; CHECK-NEXT:    s_mov_b64 s[10:11], s[2:3]
+; CHECK-NEXT:    s_mov_b64 s[8:9], s[0:1]
+; CHECK-NEXT:    s_mov_b64 s[0:1], s[8:9]
+; CHECK-NEXT:    s_mov_b64 s[2:3], s[10:11]
+; CHECK-NEXT:    s_swappc_b64 s[30:31], s[6:7]
+; CHECK-NEXT:    v_readlane_b32 s30, v2, 0
+; CHECK-NEXT:    v_readlane_b32 s31, v2, 1
+; CHECK-NEXT:    s_add_i32 s32, s32, 0xfffffc00
+; CHECK-NEXT:    v_readlane_b32 s33, v2, 2
+; CHECK-NEXT:    s_or_saveexec_b64 s[4:5], -1
+; CHECK-NEXT:    buffer_load_dword v2, off, s[0:3], s32 offset:4 ; 4-byte Folded Reload
+; CHECK-NEXT:    s_mov_b64 exec, s[4:5]
+; CHECK-NEXT:    s_waitcnt vmcnt(0)
+; CHECK-NEXT:    s_setpc_b64 s[30:31]
+entry:
+  %retval = alloca i32, align 4, addrspace(5)
+  %retval.ascast = addrspacecast i32 addrspace(5)* %retval to i32*
+  %call = call i32 @bar()
+  ret i32 %call
+}
+
+define protected amdgpu_kernel void @kernel() {
+; CHECK-LABEL: kernel:
+; CHECK:       ; %bb.0: ; %entry
+; CHECK-NEXT:    s_mov_b32 s32, 0
+; CHECK-NEXT:    s_mov_b32 flat_scratch_lo, s7
+; CHECK-NEXT:    s_add_i32 s6, s6, s9
+; CHECK-NEXT:    s_lshr_b32 flat_scratch_hi, s6, 8
+; CHECK-NEXT:    s_add_u32 s0, s0, s9
+; CHECK-NEXT:    s_addc_u32 s1, s1, 0
+; CHECK-NEXT:    s_getpc_b64 s[6:7]
+; CHECK-NEXT:    s_add_u32 s6, s6, foo@rel32@lo+4
+; CHECK-NEXT:    s_addc_u32 s7, s7, foo@rel32@hi+12
+; CHECK-NEXT:    s_mov_b64 s[10:11], s[2:3]
+; CHECK-NEXT:    s_mov_b64 s[8:9], s[0:1]
+; CHECK-NEXT:    s_mov_b64 s[0:1], s[8:9]
+; CHECK-NEXT:    s_mov_b64 s[2:3], s[10:11]
+; CHECK-NEXT:    s_swappc_b64 s[30:31], s[6:7]
+; CHECK-NEXT:    s_endpgm
+entry:
+  %call = call i32 @foo()
+  ret void
+}
+
+attributes #0 = { "frame-pointer"="none" noinline }
+attributes #1 = { "frame-pointer"="all" noinline }