[PATCH] D78811: [AMDGPU] Enable base pointer.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/docs/AMDGPUUsage.rst:3875-3884
+.. _amdgpu-amdhsa-kernel-prolog-base-pointer:
+
+Base Pointer
++++++++++++++
+
+If the kernel has function calls and there needs a base pointer for the reasons
+defined in ``SIFrameLowering`` then SGPR34 is used and is always set to ``0`` in
----------------
This isn't supposed to be an ABI visible thing, so I'm not sure if this belongs here. The note about replacing it with a 0 immediate offset doesn't make sense too


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:110-121
+    // There's no free lane to spill, and no free register to save FP/BP,
+    // so we're forced to spill another VGPR to use for the spill.
+    int NewFI = MF.getFrameInfo().CreateStackObject(4, 4, true, nullptr,
+                                                    TargetStackID::SGPRSpill);
+    if (!MFI->allocateSGPRSpillToVGPR(MF, NewFI))
+      llvm_unreachable("allocate SGPR spill should have worked");
+    FrameIndex = NewFI;
----------------
I don't think this case is stressed in the tests, but also don't think we should handle this case here. We assume all SGPR->VGPR spills have been resolved in SILowerSGPRSpills. Trying to get an additional VGPR to spill in PEI is less reliable


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:646
+  Register BasePtrReg =
+      TRI.hasBasePointer(MF) ? TRI.getBaseRegister() : AMDGPU::NoRegister;
   LivePhysRegs LiveRegs;
----------------
Register() instead of NoRegister


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:676-678
+    // Make the register live throughout the function.
+    for (MachineBasicBlock &MBB : MF)
+      MBB.addLiveIn(FuncInfo->SGPRForBPSaveRestoreCopy);
----------------
Should do a single loop over the function with the other case above (also may require .sortUniqueLiveIns)


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:885
+  if (FuncInfo->BasePointerSaveIndex) {
+    const int BasePtrFI = FuncInfo->BasePointerSaveIndex.getValue();
+
----------------
* BasePointerSaveIndex is probably the normal way to get the value


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:116
+bool SIRegisterInfo::hasBasePointer(const MachineFunction &MF) const {
+  // When we need stack realignment,we can't reference off of the stack pointer,
+  // so we reserve a base pointer.
----------------
Missing space after comma


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:122
+
+Register SIRegisterInfo::getBaseRegister() const { return AMDGPU::SGPR34; }
+
----------------
Can this go in the header or is is the register enum not defined there?


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:311
+    MCRegister BasePtrReg = getBaseRegister();
+    if (BasePtrReg) {
+      reserveRegisterTuples(Reserved, BasePtrReg);
----------------
This is a unneeded check


================
Comment at: llvm/test/CodeGen/AMDGPU/stack-realign.ll:163
 
+declare void @extern_func(<32 x i32>, i32) #0
+define void @func_call_align1024_bp_gets_vgpr_spill(<32 x i32> %a, i32 %b) #0 {
----------------
Should add a test explanation comment


================
Comment at: llvm/test/CodeGen/AMDGPU/stack-realign.ll:203
+; and that value should be further referenced to load the incoming values.
+
+; GCN-LABEL: needs_align1024_stack_args_used_inside_loop:
----------------
Can you note the BP is saved/restored in an SGPR


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78811