[PATCH] D41737: [PowerPC] Try to move the stack pointer update instruction later in the prologue and earlier in the epilogue

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

These comments are not an indication that another review is necessary, but please address them on the commit.



================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:832
+  if (FrameSize && !FI->hasFastCall() && !FI->usesPICBase() && !HasFP &&
+      !HasBP && isInt<16>(FrameSize)) {
+    const std::vector< CalleeSavedInfo > & info = MFI.getCalleeSavedInfo();
----------------
stefanp wrote:
> sfertile wrote:
> > Can we use `!isLargeFrame` here instead of `isInt<16>(FrameSize) `?
> No, they are not actually the same thing unfortunately. In the prologue we use this definition:
> `bool isLargeFrame = !isInt<16>(NegFrameSize);`
> In the epilogue we use this definition.
> `bool isLargeFrame = !isInt<16>(FrameSize);`
> But I need the two conditions to be identical in the two guards that I've set up. So, unfortunately, I can't use the isLargeFrame in the prologue. 
It would probably be good to put in a quick comment explaining that so someone doesn't change it in the future.


================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:833
+      !HasBP && isInt<16>(FrameSize)) {
+    const std::vector< CalleeSavedInfo > & info = MFI.getCalleeSavedInfo();
+    for (int i=0; i<info.size(); i++) {
----------------
Nit: no spaces in the template parameter list when there's a single one. No space after the ampersand either. Also, fix up your naming convention here and elsewhere - variables start with a capital.


================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:1366
+  // Check if we can move the stack update instruction up the epilogue
+  //  past the callee saves. This will allow restoring the lr sooner.
+  MachineBasicBlock::iterator StackUpdateLoc = MBBI;
----------------
I think more importantly - it'll allow the CSR restores to hide the latency of the move-from-LR as they're between the `mflr` and `blr`.


https://reviews.llvm.org/D41737