[PATCH] D79499: [MachineOutliner] Adding aggressive tail call outlining options.
Jessica Paquette via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed May 6 10:13:33 PDT 2020
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
More information about the llvm-commits
mailing list