[PPC] Fix two bugs in frame lowering.

[jtony]

Summary

(1) The available program storage region of the red zone to compilers is 288 bytes rather than 244 bytes (both for ABI V1.9 and ABI V2).
(2) The formula for negative number alignment calculation should be y = x & ~(n-1) rather than y = (x + (n-1)) & ~(n-1).

[inouehrs]

test/CodeGen/PowerPC/tailcall1-64.ll

2 ↗

(On Diff #102983)

Does this change relate to frame lowering?

[jtony]

test/CodeGen/PowerPC/tailcall1-64.ll

2 ↗

(On Diff #102983)

This test case is modified to make sure the changes above don't regress the TCO.

[nemanjai]

lib/Target/PowerPC/PPCFrameLowering.cpp

1877 ↗

(On Diff #102983)

How are we testing this change?

[stefanp]

lib/Target/PowerPC/PPCFrameLowering.cpp

1874 ↗

(On Diff #102983)

nit: (the stack is growning down) -> (the stack grows downward)

test/CodeGen/PowerPC/tailcall1-64.ll

12 ↗

(On Diff #102983)

nit: Should we be doing a CHECK-LABEL first?

[jtony]

Address the comments from Nemanja and Stefan.
Add one test case to test the alignment calculation change.

[nemanjai]

LGTM. If @hfinkel doesn't have any objections, I think this can go in.

[hfinkel]

In D34337#794950, @nemanjai wrote:

LGTM. If @hfinkel doesn't have any objections, I think this can go in.

So... I believe if we make this change as-is then we've dropped Darwin support. Darwin had a 224-byte red zone. I'm in favor of doing that, but I don't think we should just do it subtly like this. I'd add a check for Darwin for now (to keep the 224). Then we can rip it out with the rest of Darwin support when we do that.

lib/Target/PowerPC/PPCFrameLowering.cpp

1873 ↗

(On Diff #103368)

Please add parens here (x + (n-1)) & (n-1).

[jtony]

(1) Refactor the messy red zone guard conditions to make it more readable.
(2) Differentiate the red zone behavior for DarwinABI and Non-DarwinABI (SVR4ABI).
(3) Address comments from Hal.

[inouehrs]

lib/Target/PowerPC/PPCFrameLowering.cpp

457 ↗

(On Diff #105493)

How about adding a function Subtarget.getAvailableRedZoneSize(), which returns 288 only when icPPC64 && isSVR4ABI and 224 otherwise?

[jtony]

lib/Target/PowerPC/PPCFrameLowering.cpp

457 ↗

(On Diff #105493)

I understand why you want to add that function, but if you look at the logic (!Subtarget.isPPC64()) && (FrameSize == 0) can not nicely fit into that refactor. If everything can be fit into the getAvailableRedZoneSize() function and we just use
bool FitsInRedZone = FrameSize <= Subtarget.getAvailableRedZoneSize(), the refactoring you mentioned would be more useful.

1877 ↗

(On Diff #102983)

I added one new test cases to test the alignment calculation change.

[inouehrs]

lib/Target/PowerPC/PPCFrameLowering.cpp

457 ↗

(On Diff #105493)

isSVR4ABI && isPPC64 -> return 288
isSVR4ABI && !isPPC64 -> return 0
other -> return  224

Is this representing the above conditions? (since FrameSize is unsigned FrameSize == 0 and FrameSize <= 0 is same.)

[jtony]

Address comments from Hiroshi.

[kbarton]

LGTM. Just some minor updates in the comments.

lib/Target/PowerPC/PPCSubtarget.h

277 ↗

(On Diff #105796)

Please change the Non-DarwinABI to SVR4ABI.

test/CodeGen/PowerPC/ppc-redzone-alignment-bug.ll

2 ↗

(On Diff #105796)

Can you please replace "this issue" with the specific Review that is being fixed?

10 ↗

(On Diff #105796)

Again, instead of "the fix" specify the review this is being done under.

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

unsigned PPCFrameLowering::determineFrameLayout(MachineFunction &MF,
 
   // Get stack alignments. The frame must be aligned to the greatest of these:
   unsigned TargetAlign = getStackAlignment(); // alignment required per the ABI
   unsigned MaxAlign = MFI.getMaxAlignment(); // algmt required by data in frame
   unsigned AlignMask = std::max(MaxAlign, TargetAlign) - 1;
 
   const PPCRegisterInfo *RegInfo = Subtarget.getRegisterInfo();
 
-  // If we are a leaf function, and use up to 224 bytes of stack space,
-  // don't have a frame pointer, calls, or dynamic alloca then we do not need
-  // to adjust the stack pointer (we fit in the Red Zone).
-  // The 32-bit SVR4 ABI has no Red Zone. However, it can still generate
-  // stackless code if all local vars are reg-allocated.
-  bool DisableRedZone = MF.getFunction()->hasFnAttribute(Attribute::NoRedZone);
   unsigned LR = RegInfo->getRARegister();
-  if (!DisableRedZone &&
-      (Subtarget.isPPC64() ||                      // 32-bit SVR4, no stack-
-       !Subtarget.isSVR4ABI() ||                   //   allocated locals.
-        FrameSize == 0) &&
-      FrameSize <= 224 &&                          // Fits in red zone.
-      !MFI.hasVarSizedObjects() &&                 // No dynamic alloca.
-      !MFI.adjustsStack() &&                       // No calls.
-      !MustSaveLR(MF, LR) &&
-      !RegInfo->hasBasePointer(MF)) { // No special alignment.
+  bool DisableRedZone = MF.getFunction()->hasFnAttribute(Attribute::NoRedZone);
+  bool CanUseRedZone = !MFI.hasVarSizedObjects() && // No dynamic alloca.
+                       !MFI.adjustsStack() &&       // No calls.
+                       !MustSaveLR(MF, LR) &&       // No need to save LR.
+                       !RegInfo->hasBasePointer(MF); // No special alignment.
+
+  // Note: for PPC32 SVR4ABI (Non-DarwinABI), we can still generate stackless
+  // code if all local vars are reg-allocated.
+  bool FitsInRedZone = FrameSize <= Subtarget.getRedZoneSize();
+
+  // Check whether we can skip adjusting the stack pointer (by using red zone)
+  if (!DisableRedZone && CanUseRedZone && FitsInRedZone) {
     // No need for frame
     if (UpdateMF)
       MFI.setStackSize(0);
     return 0;
   }
 
   // Get the maximum call frame size of all the calls.
   unsigned maxCallFrameSize = MFI.getMaxCallFrameSize();
for (unsigned i = 0, e = CSI.size(); i != e; ++i) {
         MFI.setObjectOffset(FI, LowerBound + MFI.getObjectOffset(FI));
       }
     }
 
     LowerBound -= 4; // The VRSAVE save area is always 4 bytes long.
   }
 
   if (HasVRSaveArea) {
-    // Insert alignment padding, we need 16-byte alignment.
-    LowerBound = (LowerBound - 15) & ~(15);
+    // Insert alignment padding, we need 16-byte alignment. Note: for postive
+    // number the alignment formula is : y = (x + (n-1)) & (~(n-1)). But since
+    // we are using negative number here (the stack grows downward). We should
+    // use formula : y = x & (~(n-1)). Where x is the size before aligning, n
+    // is the alignment size ( n = 16 here) and y is the size after aligning.
+    assert(LowerBound <= 0 && "Expect LowerBound have a non-positive value!");
+    LowerBound &= ~(15);
 
     for (unsigned i = 0, e = VRegs.size(); i != e; ++i) {
       int FI = VRegs[i].getFrameIdx();
 
       MFI.setObjectOffset(FI, LowerBound + MFI.getObjectOffset(FI));
     }
   }
 

llvm/trunk/lib/Target/PowerPC/PPCSubtarget.h

public:
 
   bool isQPXStackUnaligned() const { return IsQPXStackUnaligned; }
   unsigned getPlatformStackAlignment() const {
     if ((hasQPX() || isBGQ()) && !isQPXStackUnaligned())
       return 32;
 
     return 16;
   }
+
+  // DarwinABI has a 224-byte red zone. PPC32 SVR4ABI(Non-DarwinABI) has no
+  // red zone and PPC64 SVR4ABI has a 288-byte red zone.
+  unsigned  getRedZoneSize() const {
+    return isDarwinABI() ? 224 : (isPPC64() ? 288 : 0);
+  }
+
   bool hasHTM() const { return HasHTM; }
   bool hasFusion() const { return HasFusion; }
   bool hasFloat128() const { return HasFloat128; }
   bool isISA3_0() const { return IsISA3_0; }
   bool useLongCalls() const { return UseLongCalls; }
   bool needsSwapsForVSXMemOps() const {
     return hasVSX() && isLittleEndian() && !hasP9Vector();
   }

llvm/trunk/test/CodeGen/PowerPC/ppc-redzone-alignment-bug.ll

+; Note the formula for negative number alignment calculation should be y = x & ~(n-1) rather than y = (x + (n-1)) & ~(n-1).
+; after patch https://reviews.llvm.org/D34337, we could save 16 bytes in the best case.
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr8 < %s | FileCheck %s -check-prefix=CHECK-BE
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr8 < %s | FileCheck %s -check-prefix=CHECK-LE
+
+define signext i32 @bar(i32 signext %ii) {
+entry:
+  %0 = tail call i32 asm sideeffect "add $0, $1, $2\0A", "=r,r,r,~{f14},~{r15},~{v20}"(i32 %ii, i32 10)
+  ret i32 %0
+; Before the fix by patch D34337:
+; stdu 1, -544(1)
+; std 15, 264(1)
+; stfd 14, 400(1)
+; stdu 1, -560(1)
+; std 15, 280(1)
+; stfd 14, 416(1)
+
+; After the fix by patch D34337:
+; CHECK-LE: stdu 1, -528(1)
+; CHECK-LE:std 15, 248(1)
+; CHECK-LE:stfd 14, 384(1)
+; CHECK-BE: stdu 1, -544(1)
+; CHECK-BE:std 15, 264(1)
+; CHECK-BE:stfd 14, 400(1)
+}
+
+define signext i32 @foo() {
+entry:
+  %call = tail call signext i32 @bar(i32 signext 5)
+  ret i32 %call
+}
+

llvm/trunk/test/CodeGen/PowerPC/svr4-redzone.ll

 ; PPC32: stwu 1, -16(1)
 
 ; PPC64-LABEL: smallstack:
 ; PPC64-NOT: stdu 1, -{{[0-9]+}}(1)
 ; PPC64: blr
 
 define i8* @bigstack() nounwind {
 entry:
- %0 = alloca i8, i32 230
+ %0 = alloca i8, i32 290
        ret i8* %0
 }
 ; PPC32-LABEL: bigstack:
-; PPC32: stwu 1, -240(1)
+; PPC32: stwu 1, -304(1)
 
 ; PPC64-LABEL: bigstack:
-; PPC64: stdu 1, -288(1)
+; PPC64: stdu 1, -352(1)

llvm/trunk/test/CodeGen/PowerPC/tailcall1-64.ll

 ; RUN: llc -relocation-model=static -verify-machineinstrs < %s -march=ppc64 -tailcallopt | grep TC_RETURNd8
+; RUN: llc -relocation-model=static -verify-machineinstrs -march=ppc64 < %s | FileCheck %s
 define fastcc i32 @tailcallee(i32 %a1, i32 %a2, i32 %a3, i32 %a4) {
 entry:
   ret i32 %a3
 }
 
 define fastcc i32 @tailcaller(i32 %in1, i32 %in2) {
 entry:
-  %tmp11 = tail call fastcc i32 @tailcallee( i32 %in1, i32 %in2, i32 %in1, i32 %in2 )    ; <i32> [#uses=1]
+  %tmp11 = tail call fastcc i32 @tailcallee( i32 %in1, i32 %in2, i32 %in1, i32 %in2 )
   ret i32 %tmp11
+; CHECK-LABEL: tailcaller
+; CHECK-NOT: stdu
+; CHECK: b tailcallee
 }