[PATCH] D79499: [MachineOutliner] Adding aggressive tail call outlining options.

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

I'm a little concerned about putting this in the generic outlining algorithm, because not every target's outliner necessarily supports tail calls.

For example, the x86 outliner doesn't support tail calls right now. I would expect that in that case we should get a warning or some debug output. I can imagine someone enabling this and then being surprised when it does nothing.

I'm not opposed to this otherwise. In general, I'd like to keep potentially target-specific things outside of the generic outlining algorithm, even if those things are pretty widespread.

Also, do you think that you could build CTMark for AArch64 at -Oz with + without this change? It would be nice to see what kind of code size savings this gives us.



================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:115-117
+    cl::desc("Like -aggressive-tail-call-outlining, but stop outlining after "
+             "outlining the tail calls (ie bail immediately): Meant for "
+             "testing purposes. (default = off)"));
----------------
More succinct:

"Only outline tail calls. Meant for testing purposes. (default = off)"


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:661
 
+  /// Used to toggle Tail Call Only mode.
+  bool OnlyTailCalls;
----------------
Why is "Tail Call Only" capitalized?

I think this could be more descriptive anyway.

E.g.

"If true, only outline tail calls. If false, outline all types of frames."


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:1131-1132
   std::string FunctionName = "OUTLINED_FUNCTION_";
+  if (OnlyTailCalls)
+    FunctionName += "AGGRESSIVE_TAIL_CALL_";
   if (OutlineRepeatedNum > 0)
----------------
Why not do this as a comment on the outlined function name?


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:1512
+  bool Changed = false;
+  if (AggressiveTailCallOutlining || AggressiveTailCallOutliningOnly) {
+    // Number to append to the current tail-call-only outlined function.
----------------
It's weird to me that you can have

```
AggressiveTailCallOutlining == false
AggressiveTailCallOutliningOnly == true
```

Intuitively, I would assume that `AggressiveTailCallOutliningOnly` implies `AggressiveTailCallOutlining`.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:6259
 
+    // If we are in OnlyTailCalls mode, ignore the presence of W30 register.
+    // This improves outlining of tail calls by one or two instructions over the
----------------
Earlier, you called this "Only Tail Calls mode". This should be consistent across the board to make grepping the source easier.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79499/new/

https://reviews.llvm.org/D79499