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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
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;
----------------
arsenm wrote:
> 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
I still think this case won't work correctly. This should probably be replaced with report_fatal_error


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:885
+  if (FuncInfo->BasePointerSaveIndex) {
+    const int BasePtrFI = FuncInfo->BasePointerSaveIndex.getValue();
+
----------------
cdevadas wrote:
> arsenm wrote:
> > * BasePointerSaveIndex is probably the normal way to get the value
> Here getValue() is required. It is of type Optional<int>.
Yes, and you can use operator * instead of getValue


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:1069
 
-  Register FrameReg = getFrameRegister(*MF);
+  Register FrameReg = hasBasePointer(*MF) && FrameInfo.isFixedObjectIndex(Index)
+                          ? getBaseRegister()
----------------
Swap the order of these checks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78811