[PATCH] D30670: [Outliner] Add tail call support

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

The code looks good to me, but this still lacks a test!



================
Comment at: include/llvm/Target/TargetInstrInfo.h:1145
   /// PredicateOperand.
-  virtual bool isPredicable(const MachineInstr &MI) const {
+  virtual bool isPredicable(MachineInstr &MI) const {
     return MI.getDesc().isPredicable();
----------------
This looks unintentional (rebaseing didn't take r296901 into account probably).


================
Comment at: lib/CodeGen/MachineOutliner.cpp:924
                             const TargetInstrInfo &TII) {
+
     for (MachineBasicBlock::iterator It = MBB.begin(), Et = MBB.end(); It != Et;
----------------
seems unrelated.


================
Comment at: lib/CodeGen/MachineOutliner.cpp:1200-1201
+
+    bool IsTailCall = LastInstr->isReturn() || LastInstr->isTerminator() ||
+                      TII.isTailCall(*LastInstr);
+
----------------
isReturn() and isTailCall() imply isTerminator() so it should be enough to check just that.


================
Comment at: lib/CodeGen/MachineOutliner.cpp:1231-1233
+    bool IsTailCall =    LastInstr->isReturn() ||
+                         LastInstr->isTerminator() ||
+                         TII.isTailCall(*LastInstr);
----------------
same as above.


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:10419-10421
+  // Is this a tail call? If yes, we can outline as a tail call.
+  if (isTailCall(MI))
+    return MachineOutlinerInstrType::Legal;
----------------
I still wonder if things would just work if you change this to:
`if (MI.isTerminator() && MI->getParent()->succ_empty())` so you can catch returns or noreturn functions such as exit() or abort() as well.

If I miss something then go ahead with the current change. I just have the feeling that one of the next commits will just start renaming all the `IsTailCall` to something like `ExitsFunction` anyway...


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:10424
+  // Is this the terminator of a basic block?
+  if (MI.isTerminator() || MI.isReturn()) {
+
----------------
isReturn() implies isTerminator().


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:10448-10458
+  // 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.
----------------
Things like improving comments can be committed right away without review (people that care can do post-commit review). This has the added benefits that the actual reviews become smaller.


https://reviews.llvm.org/D30670