[AMDGPU] Fixup use of StackPtrOffsetReg when not initialized

[dstuttard]

Summary

In some cases, StackPtrOffsetReg will not have been initialized. (In the case
where MFI.hasCalls() evaluates to false).

In this situation we can use the register, but don't need to add/subtract the
offset (it's not used in the rest of the entry function).

Change-Id: If317b29ff1e5c75a3ed3662920e6e960bca3b782

[dstuttard]

Improved the test to include one that has a call provoking s32 to be initialized

[dstuttard]

Rebase to get tests to pass

[arsenm]

A small MIR test would also be helpful

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

460–463

hasCalls seems like the wrong check here. Should there be a stackPointerIsTracked() or something to go along with hasFP?

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp

741

This shouldn't need to be an explicit property?

785

Check the theoretical stackPointerIsTracked instead?

[dstuttard]

I'm thinking that this fix may be taking the wrong approach now.

I don't think that using StackPtrOffset (s32) is going to work in all cases. I'm still investigating alternatives.

[arsenm]

I think the fundamental problem here is that we wouldn't have any register to increment/decrement in the kernel, and yet the offset is still too far to fit in the immediate field of the instruction and it also can't scavenge a register.

I think this is implicitly relying on trashing the reserved SP. If we're going to rely on this, then there's no reason to increment/decrement it since you can just assign the value directly to the register. We do still try to avoid reserving an SP, although at this point I think it's far more trouble than it's worth to try to save that one register.

The other option is to treat any frame that can possibly have an offset that won't fit in the immediate field as needing an SP

[arsenm]

I tried looking at this again and I'm somewhat confused. I'm unable to break this. Was this intended only as an optimization?

[scott.linder]

The other option is to treat any frame that can possibly have an offset that won't fit in the immediate field as needing an SP

I think this would simplify things a lot, and doesn't seem like it should have a huge effect on performance? If you are already indexing into your stack that far does it really matter that you have one fewer SGPR free?

[arsenm]

I've run into another case where SP isn't initialized with a more clear explanation which may be the same problem. We initially select buffer instructions directly referencing SP, and fix it up to the FP later in eliminateFrameIndex. However, if LocalStackSlotAllocation fires on a particular store, it no longer directly references a frame index. Therefore, eliminateFrameIndex never fixes up the scratch offset register in the downstream memory instructions so they still incorrectly reference SP. We can hack around this by forcing SP initialization if getLocalFrameObjectCount, but I think the real fix is to use stack access pseudos before we know the offset register to use

[arsenm]

Is this still needed?

[dstuttard]

I'm revisiting this - (sorry for the lengthy hiatus)

Matt - did you resolve the issues you were seeing by going with your suggestion of always initialising the SP if getLocalFrameObjectCount?

[arsenm]

There have been various frame fixes since this was initially posted. Please check if this testcase is still relevant

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

for (MCPhysReg Reg : AllSGPRs) {
         break;
       }
     }
   } else {
     ScratchWaveOffsetReg = PreloadedScratchWaveOffsetReg;
   }
   assert(ScratchWaveOffsetReg);
 
   if (MF.getFrameInfo().hasCalls()) {
+    // Note: Spilling code in SIRegisterInfo.cpp assumes that if !hasCalls() it
+    // can overwrite StackPtrOffsetReg - updates are required there if changes
+    // are made here to that assumption.
     Register SPReg = MFI->getStackPtrOffsetReg();
     assert(SPReg != AMDGPU::SP_REG);
     BuildMI(MBB, I, DL, TII->get(AMDGPU::S_MOV_B32), SPReg)
         .addImm(MF.getFrameInfo().getStackSize() * ST.getWavefrontSize());
   }
 
   if (hasFP(MF)) {
     Register FPReg = MFI->getFrameOffsetReg();

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp

void SIRegisterInfo::buildSpillLoadStore(MachineBasicBlock::iterator MI,
   const SIMachineFunctionInfo *FuncInfo = MF->getInfo<SIMachineFunctionInfo>();
 
   const MCInstrDesc &Desc = TII->get(LoadStoreOp);
   const DebugLoc &DL = MI->getDebugLoc();
   bool IsStore = Desc.mayStore();
 
   bool Scavenged = false;
   MCRegister SOffset = ScratchOffsetReg;
+  bool UninitStackPtrOffset = false;
 
   const unsigned EltSize = 4;
   const TargetRegisterClass *RC = getRegClassForReg(MF->getRegInfo(), ValueReg);
   unsigned NumSubRegs = AMDGPU::getRegBitWidth(RC->getID()) / (EltSize * CHAR_BIT);
   unsigned Size = NumSubRegs * EltSize;
   int64_t Offset = InstOffset + MFI.getObjectOffset(Index);
   int64_t ScratchOffsetRegDelta = 0;
 
if (!isUInt<12>(Offset + Size - EltSize)) {
     if (!SOffset) {
       // There are no free SGPRs, and since we are in the process of spilling
       // VGPRs too.  Since we need a VGPR in order to spill SGPRs (this is true
       // on SI/CI and on VI it is true until we implement spilling using scalar
       // stores), we have no way to free up an SGPR.  Our solution here is to
       // add the offset directly to the ScratchOffset or StackPtrOffset
       // register, and then subtract the offset after the spill to return the
       // register to it's original value.
-      if (!ScratchOffsetReg)
+      // In the case where StackPtrOffset is not initialized/otherwise used
+      // (hasCalls is false), we can just use the register directly with no
+      // adjustment required.
+      if (!ScratchOffsetReg) {
         ScratchOffsetReg = FuncInfo->getStackPtrOffsetReg();
+        UninitStackPtrOffset = !MFI.hasCalls();
+      }
       SOffset = ScratchOffsetReg;
       ScratchOffsetRegDelta = Offset;
     } else {
       Scavenged = true;
     }
 
     if (!SOffset)
       report_fatal_error("could not scavenge SGPR to spill in entry function");
 
-    if (ScratchOffsetReg == AMDGPU::NoRegister) {
+    if (ScratchOffsetReg == AMDGPU::NoRegister || UninitStackPtrOffset) {
       BuildMI(*MBB, MI, DL, TII->get(AMDGPU::S_MOV_B32), SOffset)
           .addImm(Offset);
     } else {
       BuildMI(*MBB, MI, DL, TII->get(AMDGPU::S_ADD_U32), SOffset)
           .addReg(ScratchOffsetReg)
           .addImm(Offset);
     }
 
if (!MIB.getInstr()) {
                       FinalReg)
           .addReg(TmpReg, RegState::Kill);
     }
 
     if (NumSubRegs > 1)
       MIB.addReg(ValueReg, RegState::Implicit | SrcDstRegState);
   }
 
-  if (ScratchOffsetRegDelta != 0) {
+  if (!UninitStackPtrOffset && ScratchOffsetRegDelta != 0) {
     // Subtract the offset we added to the ScratchOffset register.
     BuildMI(*MBB, MI, DL, TII->get(AMDGPU::S_SUB_U32), SOffset)
         .addReg(SOffset)
         .addImm(ScratchOffsetRegDelta);
   }
 }
 
 // Generate a VMEM access which loads or stores the VGPR containing an SGPR

llvm/test/CodeGen/AMDGPU/spill-scavenge-offset.ll

; mark most VGPR registers as used to increase register pressure
 
   %outptr = getelementptr <1280 x i32>, <1280 x i32> addrspace(1)* %out, i32 %tid
   store <1280 x i32> %a, <1280 x i32> addrspace(1)* %outptr
 
   ret void
 }
 
 ; CHECK-LABEL: test_limited_sgpr
-; GFX6: s_add_u32 s32, s32, 0x[[OFFSET:[0-9]+]]
+; GFX6: s_mov_b32 s32, 0x{{[0-9]+}}
 ; GFX6-NEXT: buffer_load_dword v{{[0-9]+}}, off, s[{{[0-9:]+}}], s32
-; GFX6-NEXT: s_sub_u32 s32, s32, 0x[[OFFSET:[0-9]+]]
 ; GFX6: NumSgprs: 48
 ; GFX6: ScratchSize: 8624
 define amdgpu_kernel void @test_limited_sgpr(<64 x i32> addrspace(1)* %out, <64 x i32> addrspace(1)* %in) #0 {
 entry:
   %lo = call i32 @llvm.amdgcn.mbcnt.lo(i32 -1, i32 0)
   %tid = call i32 @llvm.amdgcn.mbcnt.hi(i32 -1, i32 %lo)
 
 ; allocate enough scratch to go beyond 2^12 addressing

 
 ret:
   %outptr = getelementptr <64 x i32>, <64 x i32> addrspace(1)* %out, i32 %tid
   store <64 x i32> %a, <64 x i32> addrspace(1)* %outptr
 
   ret void
 }
 
+; CHECK-LABEL: test_limited_sgpr_with_call
+; GFX6: s_add_u32 s32, s32, 0x[[OFFSET:[0-9]+]]
+; GFX6-NEXT: buffer_load_dword v{{[0-9]+}}, off, s[{{[0-9:]+}}], s32
+; GFX6-NEXT: s_sub_u32 s32, s32, 0x[[OFFSET]]
+; GFX6: NumSgprs: 48
+; GFX6: ScratchSize: 24992
+define amdgpu_kernel void @test_limited_sgpr_with_call(<64 x i32> addrspace(1)* %out, i32 addrspace(1)* %call_out, <64 x i32> addrspace(1)* %in) #0 {
+entry:
+  %lo = call i32 @llvm.amdgcn.mbcnt.lo(i32 -1, i32 0)
+  %tid = call i32 @llvm.amdgcn.mbcnt.hi(i32 -1, i32 %lo)
+
+; allocate enough scratch to go beyond 2^12 addressing
+  %scratch = alloca <1280 x i32>, align 8, addrspace(5)
+
+; load VGPR data
+  %aptr = getelementptr <64 x i32>, <64 x i32> addrspace(1)* %in, i32 %tid
+  %a = load <64 x i32>, <64 x i32> addrspace(1)* %aptr
+
+; make sure scratch is used
+  %x = extractelement <64 x i32> %a, i32 0
+  %sptr0 = getelementptr <1280 x i32>, <1280 x i32> addrspace(5)* %scratch, i32 %x, i32 0
+  store i32 1, i32 addrspace(5)* %sptr0
+
+; fill up SGPRs
+  %sgpr0 = call <8 x i32> asm sideeffect "; def $0", "=s" ()
+  %sgpr1 = call <8 x i32> asm sideeffect "; def $0", "=s" ()
+  %sgpr2 = call <8 x i32> asm sideeffect "; def $0", "=s" ()
+  %sgpr3 = call <8 x i32> asm sideeffect "; def $0", "=s" ()
+  %sgpr4 = call <4 x i32> asm sideeffect "; def $0", "=s" ()
+  %sgpr5 = call <2 x i32> asm sideeffect "; def $0", "=s" ()
+  %sgpr6 = call <2 x i32> asm sideeffect "; def $0", "=s" ()
+  %sgpr7 = call i32 asm sideeffect "; def $0", "=s" ()
+
+  %cmp = icmp eq i32 %x, 0
+  br i1 %cmp, label %bb0, label %ret
+
+bb0:
+; create SGPR pressure
+  call void asm sideeffect "; use $0,$1,$2,$3,$4,$5,$6", "s,s,s,s,s,s,s,s"(<8 x i32> %sgpr0, <8 x i32> %sgpr1, <8 x i32> %sgpr2, <8 x i32> %sgpr3, <4 x i32> %sgpr4, <2 x i32> %sgpr5, <2 x i32> %sgpr6, i32 %sgpr7)
+
+; mark most VGPR registers as used to increase register pressure
+  call void asm sideeffect "", "~{v4},~{v8},~{v12},~{v16},~{v20},~{v24},~{v28},~{v32}" ()
+  call void asm sideeffect "", "~{v36},~{v40},~{v44},~{v48},~{v52},~{v56},~{v60},~{v64}" ()
+  call void asm sideeffect "", "~{v68},~{v72},~{v76},~{v80},~{v84},~{v88},~{v92},~{v96}" ()
+  call void asm sideeffect "", "~{v100},~{v104},~{v108},~{v112},~{v116},~{v120},~{v124},~{v128}" ()
+  call void asm sideeffect "", "~{v132},~{v136},~{v140},~{v144},~{v148},~{v152},~{v156},~{v160}" ()
+  call void asm sideeffect "", "~{v164},~{v168},~{v172},~{v176},~{v180},~{v184},~{v188},~{v192}" ()
+  call void asm sideeffect "", "~{v196},~{v200},~{v204},~{v208},~{v212},~{v216},~{v220},~{v224}" ()
+  br label %ret
+
+ret:
+  %outptr = getelementptr <64 x i32>, <64 x i32> addrspace(1)* %out, i32 %tid
+  store <64 x i32> %a, <64 x i32> addrspace(1)* %outptr
+
+  %call_tmp = call i32 @a_func(i32 1)
+  store volatile i32 %call_tmp, i32 addrspace(1)* %call_out
+
+  ret void
+}
+
 declare i32 @llvm.amdgcn.mbcnt.lo(i32, i32) #1
 declare i32 @llvm.amdgcn.mbcnt.hi(i32, i32) #1
+declare i32 @a_func(i32) #1
 
 attributes #0 = { "amdgpu-waves-per-eu"="10,10" }
 attributes #1 = { nounwind readnone }