[PPC] Properly update register save area offsets

[kparzysz]

Summary

The variables MinGPR/MinG8R were not updated properly when resetting the offsets, which in the included testcase lead to saving the CR register in the same location as R30.

[kparzysz]

Mark Millard was kind enough to test this patch via buildworld on FreeBSD/ppc32. See https://bugs.llvm.org/show_bug.cgi?id=26519 for details.

[nemanjai]

Do we have anything we can/want to test on 64-bit targets considering there's a change that presumably affects them (i.e. the update for the base pointer).

In any case, save for the 64-bit test case, this looks perfectly fine to me but I think I should defer the final OK to @hfinkel as he's way more familiar with this code than I am.

lib/Target/PowerPC/PPCFrameLowering.cpp

1790 ↗

(On Diff #98356)

I am frankly not familiar enough with the frame lowering code or the concept of the base pointer, but I think a comment here is in order.
I suppose that when a function has a base pointer and uses no callee-saved registers, we need a slot for the base pointer.

[kparzysz]

I don't have access to PPC hardware so there isn't much that I can do in terms of testing other than running "check-llvm".

lib/Target/PowerPC/PPCFrameLowering.cpp

1790 ↗

(On Diff #98356)

The base pointer is a "stack pointer" that is aligned to a boundary that exceeds the natural stack alignment. The code added in this patch checks if it's a 32- or 64-bit register to update the correct variable.

[nemanjai]

In D33017#754972, @kparzysz wrote:

I don't have access to PPC hardware so there isn't much that I can do in terms of testing other than running "check-llvm".

I'll test this on ppc64le and will update this patch with a comment with the results of the testing when it's done.

[nemanjai]

This passes a double bootstrap with full testing. So I think it's fine. However, are you able to add test cases for 32/64-bit targets that just saves the base pointer (i.e. a function that will use a base pointer without using any callee-saved registers and will save the base pointer - X30/R30/R29)?

[hfinkel]

In D33017#756308, @nemanjai wrote:

This passes a double bootstrap with full testing. So I think it's fine.

This looks good to me as well. Once you come up with a test as requested, or figure out that you can't, please go ahead.

However, are you able to add test cases for 32/64-bit targets that just saves the base pointer (i.e. a function that will use a base pointer without using any callee-saved registers and will save the base pointer - X30/R30/R29)?

[kparzysz]

Many thanks to both of you!

llvm/trunk/lib/Target/PowerPC/PPCFrameLowering.cpp

if (HasFPSaveArea) {
     }
 
     LowerBound -= (31 - TRI->getEncodingValue(MinFPR) + 1) * 8;
   }
 
   // Check whether the frame pointer register is allocated. If so, make sure it
   // is spilled to the correct offset.
   if (needsFP(MF)) {
-    HasGPSaveArea = true;
-
     int FI = PFI->getFramePointerSaveIndex();
     assert(FI && "No Frame Pointer Save Slot!");
-
     MFI.setObjectOffset(FI, LowerBound + MFI.getObjectOffset(FI));
+    // FP is R31/X31, so no need to update MinGPR/MinG8R.
+    HasGPSaveArea = true;
   }
 
   if (PFI->usesPICBase()) {
-    HasGPSaveArea = true;
-
     int FI = PFI->getPICBasePointerSaveIndex();
     assert(FI && "No PIC Base Pointer Save Slot!");
-
     MFI.setObjectOffset(FI, LowerBound + MFI.getObjectOffset(FI));
+
+    MinGPR = std::min<unsigned>(MinGPR, PPC::R30);
+    HasGPSaveArea = true;
   }
 
   const PPCRegisterInfo *RegInfo = Subtarget.getRegisterInfo();
   if (RegInfo->hasBasePointer(MF)) {
-    HasGPSaveArea = true;
-
     int FI = PFI->getBasePointerSaveIndex();
     assert(FI && "No Base Pointer Save Slot!");
-
     MFI.setObjectOffset(FI, LowerBound + MFI.getObjectOffset(FI));
+
+    unsigned BP = RegInfo->getBaseRegister(MF);
+    if (PPC::G8RCRegClass.contains(BP)) {
+      MinG8R = std::min<unsigned>(MinG8R, BP);
+      HasG8SaveArea = true;
+    } else if (PPC::GPRCRegClass.contains(BP)) {
+      MinGPR = std::min<unsigned>(MinGPR, BP);
+      HasGPSaveArea = true;
+    }
   }
 
   // General register save area starts right below the Floating-point
   // register save area.
   if (HasGPSaveArea || HasG8SaveArea) {
     // Move general register save area spill slots down, taking into account
     // the size of the Floating-point register save area.
     for (unsigned i = 0, e = GPRegs.size(); i != e; ++i) {

llvm/trunk/test/CodeGen/PowerPC/save-bp.ll

+; RUN: llc -march=ppc64 -ppc-always-use-base-pointer < %s | FileCheck %s --check-prefix CHECK --check-prefix PPC64
+; RUN: llc -march=ppc32 -ppc-always-use-base-pointer < %s | FileCheck %s --check-prefix CHECK --check-prefix PPC32
+; RUN: llc -march=ppc32 -ppc-always-use-base-pointer -relocation-model pic < %s | FileCheck %s --check-prefix CHECK --check-prefix PPC32PIC
+
+; CHECK-LABEL: fred:
+
+; Check for saving/restoring frame pointer (X31) and base pointer (X30)
+; on ppc64:
+; PPC64: std 31, -8(1)
+; PPC64: std 30, -16(1)
+; PPC64: ld 31, -8(1)
+; PPC64: ld 30, -16(1)
+
+; Check for saving/restoring frame pointer (R31) and base pointer (R30)
+; on ppc32:
+; PPC32: stwux 1, 1, 0
+; PPC32; addic 0, 0, -4
+; PPC32: stwx 31, 0, 0
+; PPC32: addic 0, 0, -4
+; PPC32: stwx 30, 0, 0
+; The restore sequence:
+; PPC32: lwz 31, 0(1)
+; PPC32: addic 30, 0, 8
+; PPC32: lwz 0, -4(31)
+; PPC32: lwz 30, -8(31)
+; PPC32: mr 1, 31
+; PPC32: mr 31, 0
+
+; Check for saving/restoring frame pointer (R31) and base pointer (R29)
+; on ppc32/pic. This is mostly the same as without pic, except that base
+; pointer is in R29.
+; PPC32PIC: stwux 1, 1, 0
+; PPC32PIC; addic 0, 0, -4
+; PPC32PIC: stwx 31, 0, 0
+; PPC32PIC: addic 0, 0, -8
+; PPC32PIC: stwx 29, 0, 0
+; The restore sequence:
+; PPC32PIC: lwz 31, 0(1)
+; PPC32PIC: addic 29, 0, 12
+; PPC32PIC: lwz 0, -4(31)
+; PPC32PIC: lwz 29, -12(31)
+; PPC32PIC: mr 1, 31
+; PPC32PIC: mr 31, 0
+
+
+target datalayout = "E-m:e-p:32:32-i64:64-n32"
+target triple = "powerpc-unknown-freebsd"
+
+define i64 @fred() local_unnamed_addr #0 {
+entry:
+  ret i64 0
+}
+
+attributes #0 = { norecurse readnone nounwind sspstrong "no-frame-pointer-elim"="true" "target-cpu"="ppc" }

llvm/trunk/test/CodeGen/PowerPC/save-cr-ppc32svr4.ll

+; RUN: llc -march=ppc32 -relocation-model pic < %s | FileCheck %s
+;
+; Make sure that the CR register is saved correctly on PPC32/SVR4.
+
+; CHECK-LABEL: fred:
+; CHECK: stwu 1, -32(1)
+; CHECK: stw 31, 28(1)
+; CHECK: mr 31, 1
+; CHECK: stw 30, 24(1)
+; CHECK: mfcr [[CR:[0-9]+]]
+; CHECK: stw [[CR]], 20(31)
+
+target datalayout = "E-m:e-p:32:32-i64:64-n32"
+target triple = "powerpc-unknown-freebsd"
+
+; Function Attrs: norecurse nounwind readnone sspstrong
+define i64 @fred(double %a0) local_unnamed_addr #0 {
+b1:
+  %v2 = fcmp olt double %a0, 0x43E0000000000000
+  br i1 %v2, label %b3, label %b7
+
+b3:                                               ; preds = %b1
+  %v4 = fcmp olt double %a0, 0xC3E0000000000000
+  %v5 = fptosi double %a0 to i64
+  %v6 = select i1 %v4, i64 -9223372036854775808, i64 %v5
+  br label %b14
+
+b7:                                               ; preds = %b1
+  %v8 = fcmp olt double %a0, 0x43F0000000000000
+  br i1 %v8, label %b9, label %b11
+
+b9:                                               ; preds = %b7
+  %v10 = fptoui double %a0 to i64
+  br label %b14
+
+b11:                                              ; preds = %b7
+  %v12 = fcmp ogt double %a0, 0.000000e+00
+  %v13 = sext i1 %v12 to i64
+  br label %b14
+
+b14:                                              ; preds = %b11, %b9, %b3
+  %v15 = phi i64 [ %v6, %b3 ], [ %v10, %b9 ], [ %v13, %b11 ]
+  ret i64 %v15
+}
+
+attributes #0 = { norecurse nounwind readnone sspstrong "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "stack-protector-buffer-size"="8" "target-cpu"="ppc" }

llvm/trunk/test/CodeGen/PowerPC/save-crbp-ppc32svr4.ll

+; RUN: llc -march=ppc32 -relocation-model pic < %s | FileCheck %s
+
+; CHECK-LABEL: fred
+; CHECK: stwux 1, 1, 0
+; Save R31..R29 via R0:
+; CHECK: addic 0, 0, -4
+; CHECK: stwx 31, 0, 0
+; CHECK: addic 0, 0, -4
+; CHECK: stwx 30, 0, 0
+; CHECK: addic 0, 0, -4
+; CHECK: stwx 29, 0, 0
+; Set R29 back to the value of R0 from before the updates:
+; CHECK: addic 29, 0, 12
+; Save CR through R12 using R29 as the stack pointer (aligned base pointer).
+; CHECK: mfcr 12
+; CHECK: stw 28, -16(29)
+; CHECK: stw 12, -20(29)
+
+target datalayout = "E-m:e-p:32:32-i64:64-n32"
+target triple = "powerpc-unknown-freebsd"
+
+; Function Attrs: norecurse readnone sspstrong
+define i64 @fred(double %a0) local_unnamed_addr #0 {
+b1:
+  %v2 = alloca i64, align 128
+  store i64 0, i64* %v2
+  %v3 = fcmp olt double %a0, 0x43E0000000000000
+  br i1 %v3, label %b4, label %b8
+
+b4:                                               ; preds = %b1
+  %v5 = fcmp olt double %a0, 0xC3E0000000000000
+  %v6 = fptosi double %a0 to i64
+  store i64 %v6, i64* %v2
+  %v7 = select i1 %v5, i64 -9223372036854775808, i64 %v6
+  br label %b15
+
+b8:                                               ; preds = %b1
+  %v9 = fcmp olt double %a0, 0x43F0000000000000
+  br i1 %v9, label %b10, label %b12
+
+b10:                                              ; preds = %b8
+  %v11 = fptoui double %a0 to i64
+  br label %b15
+
+b12:                                              ; preds = %b8
+  %v13 = fcmp ogt double %a0, 0.000000e+00
+  %v14 = sext i1 %v13 to i64
+  br label %b15
+
+b15:                                              ; preds = %b12, %b10, %b4
+  %v16 = phi i64 [ %v7, %b4 ], [ %v11, %b10 ], [ %v14, %b12 ]
+  %v17 = load i64, i64* %v2
+  %v18 = add i64 %v17, %v16
+  ret i64 %v18
+}
+
+attributes #0 = { norecurse readnone sspstrong "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "stack-protector-buffer-size"="8" "target-cpu"="ppc" }