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

[Quentin Colombet]

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;
----------------
qcolombet wrote:
> 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.
Do I still need to do something about the comment?

================
Comment at: lib/CodeGen/Passes.cpp:58
@@ -56,1 +57,3 @@
+static cl::opt<cl::boolOrDefault> OptimizeRegAlloc(
+    "optimize-regalloc", cl::Hidden,
     cl::desc("Enable optimized register allocation compilation path."));
----------------
clang-format related. I can back that out.

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:389-392
@@ +388,6 @@
+  MachineBasicBlock *Entry = &MF.front();
+  MachineBasicBlock *Save = MFI->getSavePoint();
+
+  if (!Save)
+    Save = Entry;
+
----------------
qcolombet wrote:
> 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.
What do you think?
Should I:
1. Keep Restore and Save as they are?
2. Create an asymmetry between their handling? (Don’t like that.)
3. Push everything into MachineFrameInfo?

================
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;
+}
+
----------------
qcolombet wrote:
> 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.
The regression tests did not show any problem with the current implementation.

http://reviews.llvm.org/D9210

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