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

[Sean Fertile via Phabricator via llvm-commits]

sfertile 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");
----------------
Xiangling_L wrote:
> If we don't expect this block is PPC64 bit only, should we remove the comment as well?
Good catch.


================
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 | \
----------------
Xiangling_L wrote:
> 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?`
Good point. I picked poor names for the related checks too becuase `PWR7`/`PWR8` makes it seem like only Power8 has the mfocrf instruction which isn't the case. Its just that the V2 abi allows saving a single field while the other ABIS do not (AFAIK). I've change the aix test to run for power4, updated the check prefixes and added a comment to help clear it up.


================
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.
+
----------------
Xiangling_L wrote:
> 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.
I see. So the problem is we don't flag the registers as having a fixed spill offset, which causes us to fall into the target independent spill/restore code which does something completely sensible but not ABI compliant. I don't want to disable spilling csrs completely because it will block a lot of other meaningful work: for example nothing in spec or lnt would compile anymore. I already have a follow on patch that fixes them that I will post once I commit this patch, so it won't be forgotten :)


================
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',
----------------
Xiangling_L wrote:
> Does this `id` difference matter? Can we use regex to replace it and combine these two lines?
Its not important now, but when I fix the callee saved gprs/fprs for AIX I expect them to end up with the same id: We are creating a stack object for the gpr csr when we should be creating a fixed stack object for it. I intend to update the test to consolidate all  the ELF/AIX checks into normal CHECKS as part of that work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74349