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

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L marked 10 inline comments as done.
Xiangling_L added inline comments.


================
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:
> 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.


================
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]
----------------
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.


================
Comment at: llvm/test/CodeGen/PowerPC/Frames-alloca.ll:2
+; RUN: llc < %s -mtriple=powerpc-unknown-linux-gnu | FileCheck %s -check-prefix=PPC32-LINUX
+; 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
----------------
ZarkoCA wrote:
> Is this testing the 64-bit ELFV1 ABI or the 64-bit ELFV2 ABI? Basically I'm asking whether this should be `powerpc64le-unknown-linux-gnu` or is this ok? 
The default ABI for 64bit BE on Linux platform is ELFV1 ABI, the which AIX ABI is similar to. So it should be fine to leave it as it is.


================
Comment at: llvm/test/CodeGen/PowerPC/Frames-alloca.ll:39
+; - Frame pointer register (r31) is saved/defined/restored.
+; - Allocated area is referrd by frame pointer with correct offset.
+; - Allocated area is 4 bytes aligned in the case of int.
----------------
ZarkoCA wrote:
> typo: should be `referred`, this happens in other test cases below
Thanks. updated it.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-frames-alloca-with-func-call.ll:47
+
+CHECK: LLVM ERROR: Handling of placing parameters on the stack is unimplemented!
----------------
ZarkoCA wrote:
> Thanks for including this test in this form.  We can update it once we enable this. 
No problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72454