[PATCH] D30797: [Outliner] Add simple stack fixup support in X86

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

This looks good, just a bunch of nitpicks.



================
Comment at: lib/Target/X86/X86InstrInfo.cpp:10418
+  switch (MI.getOpcode()) {
+    case X86::MOV64mr:
+      return MI.getOperand(0).getReg() == X86::RSP ?
----------------
We don't add extra indentation to case labels (only to the code after the case).


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:10426-10427
+                                          MachineOutlinerFixupType::NotFixable;
+    default:
+      break;
+  }
----------------
Seems unnecessary.


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:10499
 
 void X86InstrInfo::insertOutlinerEpilogue(MachineBasicBlock &MBB,
                                           MachineFunction &MF,
----------------
Maybe rename the whole `insertOutlinerEpilogue` function to `fixupPostOutline`, because well this isn't just inserting the epilogue anymore.


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:10541
                                           MachineFunction &MF,
-                                          bool IsTailCall) const {
-  return;
-}
+                                          bool IsTailCall) const {}
 
----------------
Could go in a separate cleanup commit without review.


================
Comment at: lib/Target/X86/X86InstrInfo.h:550-559
+  /// \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,
----------------
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
};
```


https://reviews.llvm.org/D30797