[PATCH] D64424: [AIX] Implement LR prolog/epilog save/restore

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:804
+      if (MBBI->getOpcode() == PPC::UPDATE_VRSAVE) {        
+        if (isAIXABI)
+          report_fatal_error("VRSAVE is unimplemented on AIX");        
----------------
sfertile wrote:
> FWIW according to the original commit that guards this for ELF it shouldn't be needed for AIX either:
> 
> ```
> commit 38d945872097549606ea62b64ded743afdab0648
> Author: Bill Schmidt <wschmidt at linux.vnet.ibm.com>
> Date:   Wed Oct 10 20:54:15 2012 +0000
> 
>     The PowerPC VRSAVE register has been somewhat of an odd beast since
>     the Altivec extensions were introduced.  Its use is optional, and
>     allows the compiler to communicate to the operating system which
>     vector registers should be saved and restored during a context switch.
>     In practice, this information is ignored by the various operating
>     systems using the SVR4 ABI; the kernel saves and restores the entire
>     register state.  Setting the VRSAVE register is no longer performed by
>     the AIX XL compilers, the IBM i compilers, or by GCC on Power Linux
>     systems.  It seems best to avoid this logic within LLVM as well.
>     
>     This patch avoids generating code to update and restore VRSAVE for the
>     PowerPC SVR4 ABIs (32- and 64-bit).  The code remains in place for the
>     Darwin ABI.
>     
>     llvm-svn: 165656
> ```
> 
It is also documented by the Assembler Language Reference for AIX version 7.2 that VRSAVE is not to be used or modified by AIX ABI-compliant programs.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:58
+  if (STI.isAIXABI())
+     return STI.isPPC64() ? 40 : 20;
   return STI.isELFv2ABI() ? 24 : 40;
----------------
The patch does not add testing for this.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:923
+  if (MustSaveCR && isAIXABI)
+    report_fatal_error("Prologue CR saving is unimplemented on AIX");
+
----------------
Minor nit: Let's treat `report_fatal_error` messages as consisting of full sentences. This one should end in a period.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-lr.ll:13
+; 32BIT: stwu 1, -64(1)
+; 32BIT: bl .foo
+; 32BIT: nop
----------------
Can we check for the stack pointer adjustment in the prologue? As it is, I am under the impression that the saved link register would be overwritten by the call.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-lr.ll:22
+; 64BIT: stdu 1, -112(1)
+; 64BIT: bl .foo
+; 64BIT: nop
----------------
Same comment.


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

https://reviews.llvm.org/D64424