[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 believe the current patch is sound and correct.  I had one test suggestion and one suggestion to use a lambda to reuse some of the cr spill logic.



================
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);
----------------
sfertile wrote:
> 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.
I think the lambda is a good idea.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1798
 
   // For 32-bit SVR4, allocate the nonvolatile CR spill slot iff the
+  // function uses CR 2, 3, or 4.
----------------
With removal of isSVR4ABI() on this code path, presumably this code behaves differently for 32-bit SVR4.  Does this comment need to be updated or is this a bug?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-crspill.ll:47
+; 32BIT-NEXT:  mtlr 0
+; 32BIT-NEXT:  mtocrf 8, 12
+; 32BIT:       blr
----------------
I like this test because it validates we apply the correct mask on the restore, though I think a test that kills all 3 cr bits is also meaningful for assembly..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74349