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

[Kristof Beyls]

Hi Quentin,

Thanks for working on this - I think shrink wrapping is a very useful optimization to have.
My comments are mostly nit picks - feel free to ignore them if they don't make sense as a result of me missing something.

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?

Thanks,

Kristof


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;
----------------
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"?

================
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)
----------------
"Since we do not reach" -> "If we do not reach"?

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:379-380
@@ +378,4 @@
+  MachineFrameInfo *MFI = MF.getFrameInfo();
+  // Visited will contain all the basic block that are in the region
+  // where the callee saved registers are alive:
+  // - Anything that is not Save or Restore -> LiveThrough.
----------------
"basic block" -> "basic blocks"

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:389-392
@@ +388,6 @@
+  MachineBasicBlock *Entry = &MF.front();
+  MachineBasicBlock *Save = MFI->getSavePoint();
+
+  if (!Save)
+    Save = Entry;
+
----------------
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?

================
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;
+}
+
----------------
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?

http://reviews.llvm.org/D9210

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