[AMDGPU] Avoid assert for saved FP

[rampitec]

Summary

With spilling into AGPRs enabled we cannot reliably predict
if we need to save FP or not. We may finally spill everything
into AGPRs and never touch stack. In this case we still may
save FP. This is deficiency but not an error, so avoid the
assert.

[arsenm]

We should stop "spilling" to AGPRs entirely. It's not a spill, it's an ordinary copy due to pressure. We just need to start using the combined VGPR+AGPR classes for values @cdevadas was working on this.

The same problem exists for SGPRs and we prune the stack objects before this

[rampitec]

In D107404#2923688, @arsenm wrote:

We should stop "spilling" to AGPRs entirely. It's not a spill, it's an ordinary copy due to pressure. We just need to start using the combined VGPR+AGPR classes for values @cdevadas was working on this.

The same problem exists for SGPRs and we prune the stack objects before this

All of that is not a reason to assert on a valid program. We also cannot prune stack objects where we create FP copy yet, we do not have spills yet. That is that heurtistic in the determineCalleeSaves to produce WillHaveFP doing that.

[rampitec]

In D107404#2923720, @rampitec wrote:

In D107404#2923688, @arsenm wrote:

We should stop "spilling" to AGPRs entirely. It's not a spill, it's an ordinary copy due to pressure. We just need to start using the combined VGPR+AGPR classes for values @cdevadas was working on this.

The same problem exists for SGPRs and we prune the stack objects before this

All of that is not a reason to assert on a valid program. We also cannot prune stack objects where we create FP copy yet, we do not have spills yet. That is that heurtistic in the determineCalleeSaves to produce WillHaveFP doing that.

Note, if spilling is replaced with the copying then the option itself will be removed and the assert updated as a result. In the meanwhile we should not assert.

[rampitec]

Ping.

[arsenm]

In D107404#2923720, @rampitec wrote:

In D107404#2923688, @arsenm wrote:

We should stop "spilling" to AGPRs entirely. It's not a spill, it's an ordinary copy due to pressure. We just need to start using the combined VGPR+AGPR classes for values @cdevadas was working on this.

The same problem exists for SGPRs and we prune the stack objects before this

All of that is not a reason to assert on a valid program. We also cannot prune stack objects where we create FP copy yet, we do not have spills yet. That is that heurtistic in the determineCalleeSaves to produce WillHaveFP doing that.

This is still just blanket disabling the assert if spill to AGPR happens to be enabled. Can't this be more fine grained?

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

963

Typo everythying

[rampitec]

In D107404#2944411, @arsenm wrote:

In D107404#2923720, @rampitec wrote:

In D107404#2923688, @arsenm wrote:

We should stop "spilling" to AGPRs entirely. It's not a spill, it's an ordinary copy due to pressure. We just need to start using the combined VGPR+AGPR classes for values @cdevadas was working on this.

The same problem exists for SGPRs and we prune the stack objects before this

All of that is not a reason to assert on a valid program. We also cannot prune stack objects where we create FP copy yet, we do not have spills yet. That is that heurtistic in the determineCalleeSaves to produce WillHaveFP doing that.

This is still just blanket disabling the assert if spill to AGPR happens to be enabled. Can't this be more fine grained?

It is too early here to say if that will happen or not. The code itself tries to predict if there will be FP.

[rampitec]

Fixed typo.

[rampitec]

Ping.

[arsenm]

I don't like just disabling the assert but @cdevadas is working on fixing this special case already

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

BuildMI(MBB, MBBI, DL, TII->get(AMDGPU::S_ADD_I32), StackPtrReg)
         .addImm(RoundedSize * getScratchScaleFactor(ST))
         .setMIFlag(MachineInstr::FrameSetup);
   }
 
   assert((!HasFP || (FuncInfo->SGPRForFPSaveRestoreCopy ||
                      FuncInfo->FramePointerSaveIndex)) &&
          "Needed to save FP but didn't save it anywhere");
 
+  // If we allow spilling to AGPRs we may have saved FP but then spill
+  // everything into AGPRs instead of the stack.
   assert((HasFP || (!FuncInfo->SGPRForFPSaveRestoreCopy &&
-                    !FuncInfo->FramePointerSaveIndex)) &&
+                    !FuncInfo->FramePointerSaveIndex) ||
+                   EnableSpillVGPRToAGPR) &&
          "Saved FP but didn't need it");
 
   assert((!HasBP || (FuncInfo->SGPRForBPSaveRestoreCopy ||
                      FuncInfo->BasePointerSaveIndex)) &&
          "Needed to save BP but didn't save it anywhere");
 
   assert((HasBP || (!FuncInfo->SGPRForBPSaveRestoreCopy &&
                     !FuncInfo->BasePointerSaveIndex)) &&

llvm/test/CodeGen/AMDGPU/save-fp.ll

+; RUN: llc -march=amdgcn -mcpu=gfx908 -verify-machineinstrs < %s | FileCheck -check-prefixes=GCN,GFX908 %s
+; RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck -check-prefixes=GCN,GFX900 %s
+
+define void @foo() {
+bb:
+  ret void
+}
+
+; FIXME: We spill v40 into AGPR, but still save and restore FP
+; which is not needed in this case.
+
+; GCN-LABEL: {{^}}caller:
+
+; GFX900:     s_mov_b32 [[SAVED_FP:s[0-9]+]], s33
+; GFX900:     s_mov_b32 s33, s32
+; GFX908-NOT: s_mov_b32 s33, s32
+; GFX900:     buffer_store_dword
+; GFX908-DAG: s_mov_b32 [[SAVED_FP:s[0-9]+]], s33
+; GFX908-DAG: v_accvgpr_write_b32
+; GCN:        s_swappc_b64
+; GFX900:     buffer_load_dword
+; GFX908:     v_accvgpr_read_b32
+; GCN:        s_mov_b32 s33, [[SAVED_FP]]
+define i64 @caller() {
+bb:
+  call void asm sideeffect "", "~{v40}" ()
+  tail call void @foo()
+  ret i64 0
+}