[Outliner] Add simple stack fixup support in X86

[paquette]

Summary

This commit adds support for simple stack fixups to the MachineOutliner pass. There are certain instructions that use, say, the stack pointer in x86-64 that can safely be outlined by modifying their offsets inside outlined functions. By making these instructions legal, it's possible to find longer sequences of instructions to outline.

As a start, commit adds support for outlining MOV64mr and MOV64rm MachineInstrs that use RSP to the x86-64 outliner.

This relies on https://reviews.llvm.org/D30670.

[MatzeB]

This looks good, just a bunch of nitpicks.

lib/Target/X86/X86InstrInfo.cpp

10418

We don't add extra indentation to case labels (only to the code after the case).

10426–10427

Seems unnecessary.

10499

Maybe rename the whole insertOutlinerEpilogue function to fixupPostOutline, because well this isn't just inserting the epilogue anymore.

10541

Could go in a separate cleanup commit without review.

lib/Target/X86/X86InstrInfo.h

550–559

The usual way is to put the doxygen comments above or behind the enum members:

/// This is my Enum.
enum MyEnum {
  /// The best choice.
  A,
  /// Also good.
  B,
  C, ///< This is a more compact alternative style to document C
};

[MatzeB]

Oh and there is no test!

[majnemer]

lib/Target/X86/X86InstrInfo.cpp

10519

spacing after switch

lib/Target/X86/X86InstrInfo.h

569

const MachineInstr &MI ?

[paquette]

This is too hard to write a test for because it only handles two instructions. It was intended as a proof of concept for the AArch64 target for the outliner which I'm about to put up. I'm just going to create a patch for that instead, since it can be tested easily there. The logic here can be used for reference.

lib/Target/X86/X86InstrInfo.h

public:
   std::pair<unsigned, unsigned>
   decomposeMachineOperandsTargetFlags(unsigned TF) const override;
 
   ArrayRef<std::pair<unsigned, const char *>>
   getSerializableDirectMachineOperandTargetFlags() const override;
 
   bool isTailCall(const MachineInstr &Inst) const override;
 
+  /// \brief Represents the type of fixup that should be done to an instruction
+  /// if one is available.
+  /// \p NotFixable instructions have no fixup.
+  /// \p FixOp0 instructions have the stack pointer as operand 0, and can be
+  /// fixed by adding 8 to their displacement.
+  /// \p FixOp5 instructions have the stack pointer as operand 5, and can be
+  /// fixed by adding 8 to their displacement.
+  enum MachineOutlinerFixupType {
+    NotFixable,
+    FixOp0,
+    FixOp5
+  };
+
   unsigned getOutliningBenefit(size_t SequenceSize,
                                size_t Occurrences,
                                bool CanBeTailCall) const override;
 
   bool isFunctionSafeToOutlineFrom(MachineFunction &MF) const override;
 
+  /// \brief Return the type of fixup that an instruction would take to be valid
+  /// post-outlining if possible. Return NotFixable if no fixup is available.
+  MachineOutlinerFixupType getPostOutliningFixup(MachineInstr &MI) const;
+
+  /// \brief Fix up outlined instructions that used the stack pointer in an
+  /// outlined function \p MF.
+  void fixupPostOutline(MachineFunction &MF) const;
+
   llvm::X86GenInstrInfo::MachineOutlinerInstrType
   getOutliningType(MachineInstr &MI) const override;
 
   void insertOutlinerEpilogue(MachineBasicBlock &MBB,
                               MachineFunction &MF,
                               bool IsTailCall) const override;
 
   void insertOutlinerPrologue(MachineBasicBlock &MBB,

lib/Target/X86/X86InstrInfo.cpp

unsigned X86InstrInfo::getOutliningBenefit(size_t SequenceSize,
   // Return the number of instructions saved by outlining this sequence.
   return NotOutlinedSize > OutlinedSize ? NotOutlinedSize - OutlinedSize : 0;
 }
 
 bool X86InstrInfo::isFunctionSafeToOutlineFrom(MachineFunction &MF) const {
   return MF.getFunction()->hasFnAttribute(Attribute::NoRedZone);
 }
 
+X86InstrInfo::MachineOutlinerFixupType
+X86InstrInfo::getPostOutliningFixup(MachineInstr &MI) const {
+  if (!isFrameStoreOpcode(MI.getOpcode()) && !isFrameLoadOpcode(MI.getOpcode()))
+    return MachineOutlinerFixupType::NotFixable;
+
+  switch (MI.getOpcode()) {
+    case X86::MOV64mr:
+      return MI.getOperand(0).getReg() == X86::RSP ?
+                                          MachineOutlinerFixupType::FixOp0 :
+                                          MachineOutlinerFixupType::NotFixable;
+    case X86::MOV64rm:
+      return MI.getOperand(5).getReg() == X86::RSP ?
+                                          MachineOutlinerFixupType::FixOp5 :
+                                          MachineOutlinerFixupType::NotFixable;
+    default:
+      break;
+  }
+
+  return MachineOutlinerFixupType::NotFixable;
+}
+
 X86GenInstrInfo::MachineOutlinerInstrType
 X86InstrInfo::getOutliningType(MachineInstr &MI) const {
 
   // Don't allow debug values to impact outlining type.
   if (MI.isDebugValue() || MI.isIndirectDebugValue())
     return MachineOutlinerInstrType::Invisible;
 
   // Is this a tail call? If yes, we can outline as a tail call.
   if (isTailCall(MI))
     return MachineOutlinerInstrType::Legal;
 
   // Is this the terminator of a basic block?
   if (MI.isTerminator() || MI.isReturn()) {
 
     // Does its parent have any successors in its MachineFunction?
     if (MI.getParent()->succ_empty())
         return MachineOutlinerInstrType::Legal;
 
     // It does, so we can't tail call it.
     return MachineOutlinerInstrType::Illegal;
   }
 
-  // Don't outline anything that modifies or reads from the stack pointer.
-  //
-  // FIXME: There are instructions which are being manually built without
-  // explicit uses/defs so we also have to check the MCInstrDesc. We should be
-  // able to remove the extra checks once those are fixed up. For example,
-  // sometimes we might get something like %RAX<def> = POP64r 1. This won't be
-  // caught by modifiesRegister or readsRegister even though the instruction
-  // really ought to be formed so that modifiesRegister/readsRegister would
-  // catch it.
-  if (MI.modifiesRegister(X86::RSP, &RI) || MI.readsRegister(X86::RSP, &RI) ||
-      MI.getDesc().hasImplicitUseOfPhysReg(X86::RSP) ||
-      MI.getDesc().hasImplicitDefOfPhysReg(X86::RSP)) 
-    return MachineOutlinerInstrType::Illegal;
-
   // Outlined calls change the instruction pointer, so don't read from it.
   if (MI.readsRegister(X86::RIP, &RI) ||
       MI.getDesc().hasImplicitUseOfPhysReg(X86::RIP) ||
       MI.getDesc().hasImplicitDefOfPhysReg(X86::RIP))
     return MachineOutlinerInstrType::Illegal;
 
   // Positions can't safely be outlined.
   if (MI.isPosition())
     return MachineOutlinerInstrType::Illegal;
 
   // Make sure none of the operands of this instruction do anything tricky.
   for (const MachineOperand &MOP : MI.operands())
     if (MOP.isCPI() || MOP.isJTI() || MOP.isCFIIndex() || MOP.isFI() ||
         MOP.isTargetIndex())
       return MachineOutlinerInstrType::Illegal;
 
+  // Does this instruction modify the stack pointer?
+  //
+  // FIXME: There are instructions which are being manually built without
+  // explicit uses/defs so we also have to check the MCInstrDesc. We should be
+  // able to remove the extra checks once those are fixed up. For example,
+  // sometimes we might get something like %RAX<def> = POP64r 1. This won't be
+  // caught by modifiesRegister or readsRegister even though the instruction
+  // really ought to be formed so that modifiesRegister/readsRegister would
+  // catch it.
+  if (MI.modifiesRegister(X86::RSP, &RI) ||
+      MI.readsRegister(X86::RSP, &RI) ||
+      MI.getDesc().hasImplicitUseOfPhysReg(X86::RSP) ||
+      MI.getDesc().hasImplicitDefOfPhysReg(X86::RSP)) {
+
+      // Can we transform this instruction post-outlining so that it will be
+      // valid?
+      if (getPostOutliningFixup(MI) != MachineOutlinerFixupType::NotFixable) {
+        // Yes, so return that we can do so.
+        return MachineOutlinerInstrType::Legal;
+      } else {
+        // No, so return that it's illegal.
+        return MachineOutlinerInstrType::Illegal;
+      }
+  }
+
   return MachineOutlinerInstrType::Legal;
 }
 
 void X86InstrInfo::insertOutlinerEpilogue(MachineBasicBlock &MBB,
                                           MachineFunction &MF,
                                           bool IsTailCall) const {
-
   // If we're a tail call, we already have a return, so don't do anything.
   if (IsTailCall)
     return;
 
   // We're a normal call, so our sequence doesn't have a return instruction.
   // Add it in.
   MachineInstr *retq = BuildMI(MF, DebugLoc(), get(X86::RETQ));
   MBB.insert(MBB.end(), retq);
+
+  // Perform any necessary fixups on the outlined function.
+  fixupPostOutline(MF);
+}
+
+void X86InstrInfo::fixupPostOutline(MachineFunction &MF) const {
+  for (MachineBasicBlock &MBB: MF) {
+    // If any instruction is something we promised we could fix, fix it.
+    for (MachineInstr &MI: MBB) {
+      switch(getPostOutliningFixup(MI)) {
+        case MachineOutlinerFixupType::NotFixable:
+          break;
+        case MachineOutlinerFixupType::FixOp0 : {
+          auto &Disp = MI.getOperand(X86::AddrDisp);
+          int64_t oldDispVal = Disp.getImm();
+          Disp.setImm(oldDispVal + 8);
+          break;
+        }
+        case MachineOutlinerFixupType::FixOp5: {
+          auto &Disp = MI.getOperand(5 + X86::AddrDisp);
+          int64_t oldDispVal = Disp.getImm();
+          Disp.setImm(oldDispVal + 8);
+          break;
+        }
+      }
+    }
+  }
 }
 
 void X86InstrInfo::insertOutlinerPrologue(MachineBasicBlock &MBB,
                                           MachineFunction &MF,
-                                          bool IsTailCall) const {
-  return;
-}
+                                          bool IsTailCall) const {}
 
 MachineBasicBlock::iterator
 X86InstrInfo::insertOutlinedCall(Module &M, MachineBasicBlock &MBB,
                                  MachineBasicBlock::iterator &It,
                                  MachineFunction &MF,
                                  bool IsTailCall) const {
   // Is it a tail call?
   if (IsTailCall) {