Page MenuHomePhabricator

[AIX] Implement LR prolog/epilog save/restore
ClosedPublic

Authored by cebowleratibm on Jul 9 2019, 8:56 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

cebowleratibm created this revision.Jul 9 2019, 8:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2019, 8:57 AM
cebowleratibm retitled this revision from AIX: Implement LR prolog/epilog save/restore to [AIX] Implement LR prolog/epilog save/restore .Jul 9 2019, 9:01 AM

FYI. You can Edit Related Revision to set the parent revision to show dependency.

I'll have to dig into the AIX ABI docs before I can review this more thoroughly but it looks good. I suggest we split out renaming of the hardcoded '8' to --> getCRSaveOffset() as a separate NFC patch and that we can land immediately.

llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
796 ↗(On Diff #208696)

minor nit: Can we just say Unsupported PPC ABI?

804 ↗(On Diff #208696)

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@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
cebowleratibm edited the summary of this revision. (Show Details)

Rebased changes after D63738 and

commit 9bd22fec0d7bee6fa32479ba090b9c89656c0a3c
Date: Fri Jul 26 14:02:17 2019 +0000

[PowerPC] Add getCRSaveOffset to improve readability. [NFC]

Other NFC change to update assertion text.

llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
804 ↗(On Diff #208696)

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.

58 ↗(On Diff #212344)

The patch does not add testing for this.

923 ↗(On Diff #212344)

Minor nit: Let's treat report_fatal_error messages as consisting of full sentences. This one should end in a period.

llvm/test/CodeGen/PowerPC/aix-lr.ll
13 ↗(On Diff #212344)

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.

22 ↗(On Diff #212344)

Same comment.

llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
62 ↗(On Diff #212344)

This patch does not add testing for this.

83 ↗(On Diff #212344)

This patch does not add testing for this.

87 ↗(On Diff #212344)

This looks like a copy/paste error (copied from line 71).

90 ↗(On Diff #212344)

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.
sfertile added inline comments.Tue, Aug 6, 10:24 AM
llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
62 ↗(On Diff #212344)

Chris and I had briefly discussed this offline and I asked that we add asserts (or errors) if we need a frame-pointer or base-pointer on AIX. We can then add the testing and remove the assert/errors in a subsequent patch

cebowleratibm marked 9 inline comments as done.Thu, Aug 8, 8:49 AM
cebowleratibm added inline comments.
llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
796 ↗(On Diff #208696)

Updated the assertion text.

804 ↗(On Diff #208696)

Noted and we should simplify this in a separate patch when we remove Darwin support.

58 ↗(On Diff #212344)

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.

87 ↗(On Diff #212344)

I'll defer this to the FP/BP follow on work.

90 ↗(On Diff #212344)

to be addressed in FP/BP follow on work.

llvm/test/CodeGen/PowerPC/aix-lr.ll
13 ↗(On Diff #212344)

"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.

cebowleratibm marked 3 inline comments as done.

Update the TOC, FP and BP offset accessors to assert for AIX. The assertions will be removed with future work that accompanied by appropriate test coverage.

LGTM.

llvm/test/CodeGen/PowerPC/aix-lr.ll
13 ↗(On Diff #212344)

I missed that this was using the update form of the instruction. Sorry for the noise.

This revision is now accepted and ready to land.Mon, Aug 12, 8:54 AM
This revision was automatically updated to reflect the committed changes.

My attempt to commit this only committed the test. I'll try again once the bots clear.