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

[Chris Bowler via Phabricator via llvm-commits]

cebowleratibm marked 9 inline comments as done.
cebowleratibm added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:796
+  assert((Subtarget.isDarwinABI() || isSVR4ABI || isAIXABI) &&
+         "Currently only Darwin, SVR4 and AIX ABIs are supported for "
+         "PowerPC.");
----------------
sfertile wrote:
> minor nit: Can we just say `Unsupported PPC ABI`?
Updated the assertion text.


================
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
> ```
> 
Noted and we should simplify this in a separate patch when we remove Darwin support.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:58
+  if (STI.isAIXABI())
+     return STI.isPPC64() ? 40 : 20;
   return STI.isELFv2ABI() ? 24 : 40;
----------------
hubert.reinterpretcast wrote:
> The patch does not add testing for this.
Because the TOC save/restore is typically handled in the glue code / nop that follows the call, I don't have a reliable way to exercise this functionality.  I believe this offset comes into play for indirect calls, however, we haven't implemented that for AIX yet.

I'll add a report_fatal_error on the code path but leave the new logic in place.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:87
 
-  // SVR4 ABI: First slot in the general register save area.
+  // SVR4/AIX ABI: First slot in the general register save area.
   return STI.isPPC64()
----------------
hubert.reinterpretcast wrote:
> This looks like a copy/paste error (copied from line 71).
I'll defer this to the FP/BP follow on work.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:90
              ? -16U
              : STI.getTargetMachine().isPositionIndependent() ? -12U : -8U;
 }
----------------
hubert.reinterpretcast wrote:
> This logic does not seem to be common for 32-bit SVR4 and 32-bit AIX. See `PPCFrameLowering::determineCalleeSaves`:
> ```
>   // Reserve stack space for the PIC Base register (R30).
>   // Only used in SVR4 32-bit.
> ```
to be addressed in FP/BP follow on work.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-lr.ll:13
+; 32BIT: stwu 1, -64(1)
+; 32BIT: bl .foo
+; 32BIT: nop
----------------
hubert.reinterpretcast wrote:
> 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.
"stwu 1, -64(1)" is the SP adjustment in the prolog.  Why do you think the saved link register would be overwritten by the call?  This code saves the LR at offset 8 from SP, then saves the SP at offset 0 from SP and decrements SP by 64.


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

https://reviews.llvm.org/D64424