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

[Chris Bowler via Phabricator via llvm-commits]

cebowleratibm added a comment.

I would like to see some test coverage for the AIX CRSave assembly, whether we adapt the tests you've already updated or add a separate test.  It seems 64-bit PPC Linux has a better way of handling a single CR save.  If we choose not to use the same approach on AIX I'd like to understand why.



================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:763
+  // AIX assembler does not support cfi directives.
+  bool needsCFI = MF.needsFrameMoves() && !Subtarget.isAIXABI();
 
----------------
const


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:944
     // latency compares to mfcr.
-    unsigned MfcrOpcode = PPC::MFCR8;
-    unsigned CrState = RegState::ImplicitKill;
     if (isELFv2ABI && MustSaveCRs.size() == 1) {
+      MachineInstrBuilder MIB =
----------------
Suggest "assert(IsPPC64)" here.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:946
+      MachineInstrBuilder MIB =
+          BuildMI(MBB, MBBI, dl, TII.get(PPC::MFOCRF8), TempReg);
+      MIB.addReg(MustSaveCRs[0], RegState::Kill);
----------------
Why do we use MFOCRF8 for 64-bit Linux with 1 CR save but not on AIX?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:971
+      MachineInstrBuilder MIB =
+          BuildMI(MBB, MBBI, dl, TII.get(PPC::MFOCRF8), TempReg);
+      MIB.addReg(MustSaveCRs[0], RegState::Kill);
----------------
I can't help but notice this block of logic is redundant to the block at 946.  Can this be commoned under an "if (MustSaveCR)" block?


================
Comment at: llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp:932
+  // For 64-bit ELF and AIX, the CR save area is in the linkage area at SP+8,
+  // for 32-bit AIX thhe CR save area is in the linkage area at SP+4.
+  // We have created a FrameIndex to that spill slot to keep the CalleSaveInfos
----------------
"thhe"


================
Comment at: llvm/test/CodeGen/PowerPC/aix32-crsave.mir:1
+# RUN: llc -mtriple powerpc-unknown-aix-xcoff -x mir -mcpu=pwr4 \
+# RUN: -run-pass=prologepilog --verify-machineinstrs < %s | \
----------------
I'd rather see an aix-crsave that handles both 32/64 or one crsave test that handles all subtargets.


================
Comment at: llvm/test/CodeGen/PowerPC/aix32-crsave.mir:21
+    ; CHECK:     fixedStack:
+    ; CHECK-NOT: stack:
+    ; CHECK:      - { id: 0, type: default, offset: 4, size: 4, alignment: 4, stack-id: default,
----------------
Can you use CHECK-LABEL to ensure the expected block binds to the intended section?  


================
Comment at: llvm/test/CodeGen/PowerPC/ppc64-alloca-crspill.ll:4
+
+; RUN: llc -mtriple=powerpc64-unknown-aix-xcoff -mcpu=pwr4 \
+; RUN: --verify-machineinstrs --mattr=-altivec -stop-after=prologepilog < %s | \
----------------
I think it makes sense to change this to "alloca-crspill.ll" and add the 32-bit expected output for AIX.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74349