[PATCH] D36721: [MachineOutliner] AArch64: Avoid saving + restoring LR if possible
Jessica Paquette via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 23 10:40:09 PDT 2017
paquette added inline comments.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:883
+ size_t SequenceOverhead = StringLen;
+
----------------
davide wrote:
> I wonder why this (and other variables) are `size_t` and not just unsigned?
Mostly for consistency with the definition of length + size. I was thinking that because an "overhead" in this sense is the length of a sequence of instructions, it'd be more consistent to use size_t.
OTOH, having a std::pair<size_t, unsigned> is kind of weird, and "overhead" need not be considered a "length". I'll swap size_t out for unsigned.
================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:4443
+
+bool AArch64InstrInfo::canOutlineWithoutLRSave(MachineBasicBlock &MBB) const {
+ // Was LR saved in the function containing this basic block?
----------------
MatzeB wrote:
> Not sure if I miss something, but I would expect that it is possible to check `LR` liveness exactly at the position where the call will be inserted rather than requiring the whole block to have `LR` unused?
Oh, I think you're right. I thought that you'd have to ensure that at least the sequence is safe, but the outliner doesn't ever touch LR. That is much simpler. :)
================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:4472-4484
+unsigned AArch64InstrInfo::getOutliningCallOverhead(unsigned CallClass) const {
+ unsigned Overhead = 1;
+
+ switch (CallClass) {
+ default:
+ llvm_unreachable("Unknown call class!");
+ case MachineOutlinerDefault:
----------------
davide wrote:
> Is your cost model for the call overhead documented somewhere? If not, can you add a comment here?
I'll add a comment for sure.
There should also be more comments about how the targets interact with the outliner in the main pass, so I'll do that too.
================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:4530-4535
+ for (const auto &Interval : CandidateClass) {
+ if (!canOutlineWithoutLRSave(*(Interval.first->getParent())))
+ return std::make_pair(1, MachineOutlinerDefault);
+ }
+ return std::make_pair(1, MachineOutlinerNoLRSave);
----------------
davide wrote:
> Have you considered rewriting this code in terms of `llvm::any_of` (or `all_of`) for clarity?
... I don't know a ton of C++ magic, so you have just changed my life.
https://reviews.llvm.org/D36721
More information about the llvm-commits
mailing list