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

[Quentin Colombet]

Hi Kristof,

Thanks for your feedbacks.

> Overall, I guess that this needs support in at least one backend, so regression tests can be added before this is in a state ready to commit?


Whatever people prefers. I have a patch to add the support for shrink-wrapping for AArch64, I can just merge it with this one with the path disabled by default.

Cheers,
-Quentin


REPOSITORY
  rL LLVM

================
Comment at: lib/CodeGen/MachineFunction.cpp:626-627
@@ +625,4 @@
+
+  // Starting from MBB, check if there is a path leading to Save than do
+  // not cross Restore.
+  SmallPtrSet<const MachineBasicBlock *, 8> Visited;
----------------
kristof.beyls wrote:
> I don't understand what "... than do not cross Restore." means here.
> Should the comment just be "Starting from MBB, check if there is a patch leading to Save"?
That is a typo indeed, this is “that do not cross Restore” :).
This is the important part in fact. Restore is the kill point of the region that needs CSR to be saved and past the kill point, there is nothing to track.

================
Comment at: lib/CodeGen/MachineFunction.cpp:634-637
@@ +633,6 @@
+    const MachineBasicBlock *CurBB = WorkList.pop_back_val();
+    // By construction, the region that is after the save point is
+    // dominated by the Save and post-dominated by the Restore.
+    // Since we do not reach Restore and still reach Save, this
+    // means MBB is before Save.
+    if (CurBB == Save)
----------------
kristof.beyls wrote:
> "Since we do not reach" -> "If we do not reach"?
That works too!

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:389-392
@@ +388,6 @@
+  MachineBasicBlock *Entry = &MF.front();
+  MachineBasicBlock *Save = MFI->getSavePoint();
+
+  if (!Save)
+    Save = Entry;
+
----------------
kristof.beyls wrote:
> I'm wondering if the logic overall would get simpler if MFI->getSavePoint() would always return a MachineBasicBlock,
> i.e. returning Entry if shrink wrapping didn't change the default save point?
> That way, the logic in most functions doesn't need to handle the case separately of whether or not the save block
> and entry block are the same - generalizing the logic a bit more.
> But maybe there's a good reason to do it like this that I haven't noticed?
The rational was that this is homogenous with the way we handle the Restore point. I.e., current shrink-wrapping just give Restore point, whereas PEI can have multiple of them. So I did not want to unbalanced this.

I guess I can also push the list of exits blocks in MFI if you think that is better.

================
Comment at: lib/CodeGen/ShrinkWrap.cpp:186-210
@@ +185,27 @@
+
+bool ShrinkWrap::useOrDefCSROrFI(const MachineInstr &MI) const {
+  if (MI.getOpcode() == FrameSetupOpcode ||
+      MI.getOpcode() == FrameDestroyOpcode) {
+    DEBUG(dbgs() << "Frame instruction: " << MI << '\n');
+    return true;
+  }
+  for (const MachineOperand &MO : MI.operands()) {
+    bool UseCSR = false;
+    if (MO.isReg()) {
+      unsigned PhysReg = MO.getReg();
+      if (!PhysReg)
+        continue;
+      assert(TargetRegisterInfo::isPhysicalRegister(PhysReg) &&
+             "Unallocated register?!");
+      UseCSR = RCI.getLastCalleeSavedAlias(PhysReg);
+    }
+    // TODO: Handle regmask more accurately.
+    // For now, be conservative about them.
+    if (UseCSR || MO.isFI() || MO.isRegMask()) {
+      DEBUG(dbgs() << "Use or define CSR or FI: " << MI << '\n');
+      return true;
+    }
+  }
+  return false;
+}
+
----------------
kristof.beyls wrote:
> This seems like the most critical function to get right from a correctness point-of-view?
> I can't derive from just staring at this code whether this is also going to catch code generated for VLAs,
> or inline assembly fragments touching the frame, or registers such as stack pointer, frame pointer, base pointer.
> 
> I'm guessing this could be good cases to add as regression tests - i.e. a case where only the presence of a VLA prevents the optimization & a case where only the presence of a piece of inline assembly prevents the optimization?
If the callee-saved information is properly set, it should… Not sure how the VLA are handled, definitely a good regression test to add!
For inline assembly, I guess you are right and we could play it safe.

Additionally, we could add a target hook to check whether or not a given instruction should be after the prologue and before the epilogue. At the moment, I had needed this but if it proves to be useful we can definitely do that.

http://reviews.llvm.org/D9210

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/