[PATCH] D79252: [PowerPC][AIX] Spill CSRs to the ABI specified stack offsets.

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1836
   // RestoreBlock. So we handle this case here.
   if (MFI.getSavePoint() && MFI.hasTailCall()) {
     MachineBasicBlock *RestoreBlock = MFI.getRestorePoint();
----------------
cebowleratibm wrote:
> Since we're enabling this path for AIX is there a LIT test that we should add an AIX variant for to validate this shrink-wrap functionality?
Shrink-wrapping is only enabled for 64-bit ELF for now. I intend to enable it eventually, but not in this patch,

```
bool PPCFrameLowering::enableShrinkWrapping(const MachineFunction &MF) const {
  if (MF.getInfo<PPCFunctionInfo>()->shrinkWrapDisabled())
    return false;
  return (MF.getSubtarget<PPCSubtarget>().isSVR4ABI() &&
          MF.getSubtarget<PPCSubtarget>().isPPC64());
}
```

I can add atest where we shrink wrap for ELF 64-bit and then check we don't shrink wrap for AIX if you want, but I didn't think it was necessary. 


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:2000
 
-    if (Subtarget.isPPC64()) {
-      LowerBound -= (31 - MinReg + 1) * 8;
-    } else {
-      LowerBound -= (31 - MinReg + 1) * 4;
-    }
+    unsigned GPRegSize = Subtarget.isPPC64() ? 8 : 4;
+    LowerBound -= (31 - MinReg + 1) * GPRegSize;
----------------
cebowleratibm wrote:
> const or fold it into the use.  This change is NFC and could be committed independently.
Good point, will update and commit separately.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-byval-mem.ll:206
 
-; ASM32BIT:       stwu 1, -96(1)
+; ASM32BIT:       stwu 1, -112(1)
 ; ASM32BIT-DAG:   lwz [[REG:[0-9]+]], LC{{[0-9]+}}(2)
----------------
cebowleratibm wrote:
> Why do we expect this change to bump the stack adj?  I thought the cc tests don't touch non-volatile regs.
Some of them do: For example in this test, we load the address of the global //before// the call to memcpy. This means the compiler has to use a non-volatile (r30) to keep the value live across the call. By saving r30 to its ABI specified offset, we increase the stack frame size by 4, and to keep the stack aligned the size has to grow by 16. It's similar case for the other tests that change.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-csr.ll:176-182
+declare double @dummy(i32 signext);
+
+define dso_local double @fprs_and_gprs(i32 signext %i) {
+  call void asm sideeffect "", "~{r13},~{r14},~{r25},~{r31},~{f14},~{f19},~{f21},~{f31}"()
+  %result = call double @dummy(i32 signext %i)
+  ret double %result
+}
----------------
ZarkoCA wrote:
> What are the other tests in this file showing that this one doesn't?  It seems to me this test makes at least the `gprs_only` test redundant.
The gprs_only test shows that we calculate the offsets to gprs when he fpr save area is non-existent. The fpr only test is redundant though, I'll remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79252