[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:946
+      MachineInstrBuilder MIB =
+          BuildMI(MBB, MBBI, dl, TII.get(PPC::MFOCRF8), TempReg);
+      MIB.addReg(MustSaveCRs[0], RegState::Kill);
----------------
cebowleratibm wrote:
> Why do we use MFOCRF8 for 64-bit Linux with 1 CR save but not on AIX?
Because I'm not sure if its safe to do so on AIX. The V2 ELF ABI specifically allows saving only the clobbered crfields, the V1 ELF ABI does not. I can't find any documentation for AIX that specifies whether its safe to save only the clobbered  fields, or if CR2,CR3 and CR4 must all be saved. If we are allowed to save a single field but save the entire register anyway we are pessimistically correct, if we have to save all 3 non-volatile cr fields and we only save the clobbered ones then our codegen is incorrect, so unless we can verify using 'mfocrf' is allowed on AIX we have to match the ELF V1 implementation.

FWIW I tested out clobbering a CR field with both XL and gcc on AIX and both use 'mfcr`, so I strongly  doubt we can save only the clobbered non-volatile fields.


================
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);
----------------
cebowleratibm wrote:
> 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?
I don't think so because of the ordering with respect to the 'mflr' instruction inserted at line 961 above. I could pull the code out into a static helper or lambda if you like though.


================
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 | \
----------------
cebowleratibm wrote:
> I'd rather see an aix-crsave that handles both 32/64 or one crsave test that handles all subtargets.
I added this test to mirror the ppc64 test. I can't handle both 32-bit and 64-bit targets with the same input mir. I think its important to have a test with mir as input which runs only the prologepilog pass to highlight exactly what is expected from the pass. I've added a separate IR to assembly test that exercises both 32-bit and 64-bit codegen on AIX.


================
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,
----------------
cebowleratibm wrote:
> Can you use CHECK-LABEL to ensure the expected block binds to the intended section?  
Good suggestion. updated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74349