[PATCH] D72454: [AIX] Enable frame pointer for AIX and add related test suite

[Chris Bowler via Phabricator via llvm-commits]

cebowleratibm added a comment.

Preliminary comments.  I still need to go through the test changes more thoroughly.



================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:63
 static unsigned computeFramePointerSaveOffset(const PPCSubtarget &STI) {
   // For the Darwin ABI:
   // We cannot use the TOC save slot (offset +20) in the PowerPC linkage area
----------------
ZarkoCA wrote:
> Xiangling_L wrote:
> > ZarkoCA wrote:
> > > The comment talks about the Darwin ABI but it looks that part of the code is removed.  Should the comment be edited or am I misunderstanding? 
> > It is because we have the same logic for SVR4, AIX and Darwin, so I skip the `isDarwinABI()` check. Maybe I can adjust the structure of the comments like:
> > 
> > ```
> > Darwin comments...;
> > SVR4 ABI/AIX ABI:...;
> > 
> > return STI.isPPC64() ? -8U : -4U;  
> > 
> > ```
> > 
> > Please let me know if you think it would help.
> Yes, I think that's better thanks. 
I think it's ok to start removing darwin logic.  I think the comment block should be discarded.


================
Comment at: llvm/lib/Target/PowerPC/PPCSubtarget.h:301
+  //    the red zone. The stack floor is at 220 bytes for 32-bit mode and 288
+  //    bytes for 64-bit mode lower than the stack pointer.
+  //    [32-bit mode]
----------------
Xiangling_L wrote:
> ZarkoCA wrote:
> > Can this be rewritten to: 
> > `On the AIX ABI, the scratch area is called the stack floor as opposed to the red zone.  The stack floor is 220 bytes lower than the stack pointer in 32-bit mode and 288 bytes lower in 64-bit mode`?  Does this capture the same meaning? 
> Yes, I think so. I will adjust the comment based on your suggestion. Thanks.
I think this comment is unnecessary because it does not add anything beyond what is readily discerned from the code.  What is useful is how the stack floor / redzone is calculated.


================
Comment at: llvm/lib/Target/PowerPC/PPCSubtarget.h:307
   unsigned  getRedZoneSize() const {
-    return isDarwinABI() ? 224 : (isPPC64() ? 288 : 0);
+    if (isAIXABI())
+      return isPPC64() ? 288: 220;
----------------
Suggestion to pull the comments into the logic:

  unsigned getRedZoneSize() const {
    if (isDarwinABI())
      return 224;
    if (isPPC64())
      // 288 bytes = 18*8 (FPRs) + 18*8 (GPRs, GPR13 reserved)
      return 288;
    // AIX PPC32: 220 bytes = 18*8 (FPRs) + 19*4 (GPRs)
    // PPC32 SVR4ABI has no redzone.
    return isAIXABI() ? 220 : 0;
  }


================
Comment at: llvm/test/CodeGen/PowerPC/Frames-alloca.ll:3
+; RUN: llc < %s -mtriple=powerpc64-unknown-linux-gnu | FileCheck %s -check-prefix=PPC64-LINUX
+; RUN: llc < %s -mtriple=powerpc-unknown-linux-gnu -frame-pointer=all | FileCheck %s -check-prefix=PPC32-LINUX
+; RUN: llc < %s -mtriple=powerpc64-unknown-linux-gnu -frame-pointer=all | FileCheck %s -check-prefix=PPC64-LINUX
----------------
Why did you drop the -NOFP from the prefix?  Is the output expected to be the same between default and -frame-pointer=all?


================
Comment at: llvm/test/CodeGen/PowerPC/Frames-alloca.ll:5
+; RUN: llc < %s -mtriple=powerpc64-unknown-linux-gnu -frame-pointer=all | FileCheck %s -check-prefix=PPC64-LINUX
+; RUN: llc < %s -mtriple=powerpc-unknown-linux-gnu | FileCheck %s -check-prefix=PPC32-LINUX
+
----------------
isn't this redudant to line 1?


================
Comment at: llvm/test/CodeGen/PowerPC/aix64-frames-stack-floor.ll:1
+; RUN: llc -verify-machineinstrs < %s -mcpu=pwr4 -mattr=-altivec \
+; RUN: -mtriple=powerpc64-ibm-aix-xcoff | FileCheck %s -check-prefix=PPC64-NOFP
----------------
Why isn't there a PPC32 variant of this test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72454