[PATCH] D74349: [PowerPC][AIX] Spill and restore the non-volatile condition register bits.
Xiangling Liao via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 13 13:38:40 PST 2020
Xiangling_L added inline comments.
================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1601
if (MustSaveCR && !(SingleScratchReg && MustSaveLR)) {
// This will only occur for PPC64.
assert(RBReg == SPReg && "Should be using SP as a base register");
----------------
If we don't expect this block is PPC64 bit only, should we remove the comment as well?
================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1659
if (MustSaveCR &&
!(SingleScratchReg && MustSaveLR)) // will only occur for PPC64
for (unsigned i = 0, e = MustSaveCRs.size(); i != e; ++i)
----------------
same comment as above
================
Comment at: llvm/test/CodeGen/PowerPC/ppc64-crsave.mir:10
+
+# RUN: llc -mtriple powerpc64-unknown-aix-xcoff -x mir -mcpu=pwr7 \
+# RUN: -run-pass=prologepilog --verify-machineinstrs < %s | \
----------------
sfertile wrote:
> Xiangling_L wrote:
> > Usually, the default cpu level we set in the testcase is `pwr4`, can I ask why we want to test `pwr7` here instead?
> Its just to match the ELF V1 runstep. There should be no particular difference between pwr4 and pwr7 for this test.
I see. I slightly prefer checking `pwr4` here though. Because if we are not consistent with our default cpu level, people review this in the future may have same doubt as me `Is pwr7 triggering anything special in this testcase?`
================
Comment at: llvm/test/CodeGen/PowerPC/ppc64-crsave.mir:15
+# TODO FIXME: We only check the save and restores of the callee saved gpr for
+# ELF becuase AIX callee saved registers haven't been properly implemented yet.
+
----------------
sfertile wrote:
> Xiangling_L wrote:
> > In case we might forget to update the testcase when we support AIX, can we leave an assertion in the code instead?
> In what code?
Sorry, I thought there was some boilerplate of `save and restores of the callee saved gpr for AIX` based on `properly implement`, bt it seems I am wrong.
================
Comment at: llvm/test/CodeGen/PowerPC/ppc64-crsave.mir:34
+ ; ELF: - { id: 1, type: default, offset: 8, size: 4, alignment: 8, stack-id: default,
+ ; AIX: - { id: 0, type: default, offset: 8, size: 4, alignment: 8, stack-id: default,
+ ; CHECK: isImmutable: true, isAliased: false, callee-saved-register: '$cr4',
----------------
Does this `id` difference matter? Can we use regex to replace it and combine these two lines?
================
Comment at: llvm/test/CodeGen/PowerPC/ppc64-crsave.mir:70
+ ; CHECK-LABEL: fixedStack:
+ ; ELF: - { id: 1, type: default, offset: 8, size: 4, alignment: 8, stack-id: default,
+ ; AIX: - { id: 0, type: default, offset: 8, size: 4, alignment: 8, stack-id: default,
----------------
Same comment as above
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74349/new/
https://reviews.llvm.org/D74349
More information about the llvm-commits
mailing list