[PATCH] D74349: [PowerPC][AIX] Spill and restore the non-volatile condition register bits.

[Xiangling Liao via Phabricator via llvm-commits]

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