[PATCH] Add a shrink-wrapping pass to improve the placement of prologue and epilogue.

[Kristof Beyls]

LGTM now!

> -----Original Message-----
> From: Quentin Colombet [mailto:qcolombet at apple.com]
> Sent: 05 May 2015 17:32
> To: reviews+D9210+public+3e03982b1cc14f6b at reviews.llvm.org
> Cc: grosbach at apple.com; kparzysz at codeaurora.org; Kristof Beyls;
> dberlin at dberlin.org; Amara Emerson; justin.holewinski at gmail.com; llvm-
> commits at cs.uiuc.edu
> Subject: Re: [PATCH] Add a shrink-wrapping pass to improve the placement
> of prologue and epilogue.
> 
> Hi Kristof,
> 
> > On May 5, 2015, at 2:30 AM, Kristof Beyls <kristof.beyls at arm.com>
> wrote:
> >
> > Overall, it seems you've made a lot of changes in the Hexagon backend.
> I guess these are necessary to make the backend structured more like the
> other backends so that the SaveBlock/RestoreBlock changes in PEI can be
> done?
> 
> No, I haven’t done anything special to the Hexagon backend. The diff in
> phabricator came as part of the rebase to ToT. If you look at the raw
> diff by downloading the patch, you would see that there is nothing
> special here.
> 
> I reckon this is unfortunate how phabricator behaves on this aspect.
> 
> > If so, are the changes basically refactoring, i.e. moving existing
> code to be placed behind certain interfaces, or did you need to write a
> lot of new code in the Hexagon backend that should be reviewed in
> detail?
> >
> >
> > ================
> > Comment at: lib/CodeGen/PrologEpilogInserter.cpp:142-157
> > @@ -142,11 +141,18 @@
> > +
> > +  // Use the points found by shrink-wrapping, if any.
> > +  if (MFI->getSavePoint()) {
> > +    SaveBlock = MFI->getSavePoint();
> > +    assert(MFI->getRestorePoint() && "Both restore and save must be
> set");
> > +    RestoreBlocks.push_back(MFI->getRestorePoint());
> >     return;
> > +  }
> >
> >   // Save refs to entry and return blocks.
> > -  EntryBlock = Fn.begin();
> > +  SaveBlock = Fn.begin();
> >   for (MachineFunction::iterator MBB = Fn.begin(), E = Fn.end();
> >        MBB != E; ++MBB)
> >     if (isReturnBlock(MBB))
> > -      ReturnBlocks.push_back(MBB);
> > +      RestoreBlocks.push_back(MBB);
> >
> >   return;
> > }
> > ----------------
> > Just a minor comment, feel free to ignore this one:
> > I feel that it would be a slightly better separation of concerns if
> MFI->getSavePoint() would always be set, and PEI doesn't need to
> calculate a SavePoint itself when it isn't set.
> > For this to happen, the ShrinkWrap pass should probably always be run,
> > and when it's "switched off be default", it would just set MFI-
> >SavePoint and MFI->Restore to be equal to FN.begin() and to the
> ReturnBlocks respectively.
> > I.o.w., The second half of the code above would be executed in the
> ShrinkWrap pass.
> > If you'd push ahead and implement it like this, the name of the pass
> > also should be changed to "SaveRestorePointInserter/Calculator" or
> something similar. It's then a pass which always calculates the most
> appropriate Save and Restore blocks and happens to implement a
> ShrinkWrapping optimization.
> > As I said, maybe this is pushing for too much separation of concerns -
> I'm not sure what the negative consequences would be to require the
> "SaveRestorePointInserterPass" to always be run, if any.
> 
> I could do that, but that would imply that the MachineFrameInfo would
> have to carry a vector of restore block instead of just one pointer.
> This is a slight memory foot print change.
> I do not feel strongly about it.
> 
> >
> > ================
> > Comment at: lib/CodeGen/PrologEpilogInserter.cpp:784-785
> > @@ -732,1 +783,4 @@
> > +  // Add epilogue to restore the callee-save registers in each
> exiting block.
> > +  for (MachineBasicBlock *RetBlock : RestoreBlocks)
> > +    TFI.emitEpilogue(Fn, *RetBlock);
> >
> > ----------------
> > s/RetBlock/RestoreBlock/?
> > The basic blocks iterated through are "Restore blocks", not
> necessarily "Return blocks”?
> 
> Nice catch.
> 
> >
> > ================
> > Comment at: lib/Target/ARM/ARMFrameLowering.cpp:1692-1693
> > @@ -1690,4 +1691,4 @@
> >           unsigned Reg = UnspilledCS1GPRs[i];
> >           // Don't spill high register if the function is thumb
> >           if (!AFI->isThumbFunction() ||
> >               isARMLowRegister(Reg) || Reg == ARM::LR) {
> > ----------------
> > It's unclear to me why this change is needed?
> > If only implementing the AArch64-backend-specific functionality, how
> come you need to make a change in the AArch32-backend?
> 
> Rebase fun, not mine :).
> 
> Cheers,
> -Quentin
> 
> >
> > http://reviews.llvm.org/D9210
> >
> > EMAIL PREFERENCES
> >  http://reviews.llvm.org/settings/panel/emailpreferences/
> >
> >