[llvm] [PPC][AIX] Save/restore r31 in prolog/epilog when using base pointer (PR #100182)

[Chen Zheng via llvm-commits]

chenzheng1030 wrote:

> I know this PR has already been approved but would it be possible to instead update the initialization of `HasFP` to include the `Subtarget.isAIXABI()) && RegInfo->hasBasePointer(MF)`, as well as modify the `needsFP` helper function to take `RegInfo` and Subtarget as arguments and put the check of `RegInfo->hasBasePointer(MF) && Subtarget.isAIXABI())` into its body? My worry is that under refactoring or updating we could add a new check that missies the AIX condition. having them in the init/helper-body removes that possibility.

Thanks for taking a look. It is always good to do more discussion : )

For your recommendation, @syzaara did exactly same thing in her first commit, see https://github.com/llvm/llvm-project/pull/100182/commits/e5fd2aa95be1dc8ba62f2cbff925a5e9a59a1673 .

I suggested we do not backup/restore the `r31` by treating `r31` as the frame pointer. For this case, we don't actually need `FP`, what we need here is to just backup/restore the `r31`. Treating it as frame pointer has at least two below negative impacts for now:
- a redundant copy from `r1` to `r31`. (frame pointer needs to be initialized with `r1`.)
- `r31` is not allocatable, see `getReservedRegs()`

I agree the new change is hard to maintain. I took another look. Since backup/restore `r31` through the `emitPrologue()/emitEpilogue()` path needs to change many places and hard to maintain, can we backup/restore `r31` through the normal csr  backup/restore path, like:
```diff
diff --git a/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp b/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
index 1963582ce686..2f787e5c731d 100644
--- a/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
+++ b/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
@@ -2025,8 +2025,18 @@ void PPCFrameLowering::determineCalleeSaves(MachineFunction &MF,
   // code. Same goes for the base pointer and the PIC base register.
   if (needsFP(MF))
     SavedRegs.reset(isPPC64 ? PPC::X31 : PPC::R31);
-  if (RegInfo->hasBasePointer(MF))
+  if (RegInfo->hasBasePointer(MF)) {
     SavedRegs.reset(RegInfo->getBaseRegister(MF));
+    // On AIX, when BaseRegister(R30) is used, need to spill r31 too to match
+    // AIX trackback table requirement.
+    if (!needsFP(MF) && !SavedRegs.test(isPPC64 ? PPC::X31 : PPC::R31) &&
+        Subtarget.isAIXABI()) {
+      assert(
+          (RegInfo->getBaseRegister(MF) == (isPPC64 ? PPC::X30 : PPC::R30)) &&
+          "Invalid base register on AIX!");
+      SavedRegs.set(isPPC64 ? PPC::X31 : PPC::R31);
+    }
+  }
```

(This patch is not well tested!)

Sorry, @syzaara for the solution change...

What do you guys think? 

https://github.com/llvm/llvm-project/pull/100182