[PATCH] D36721: [MachineOutliner] AArch64: Avoid saving + restoring LR if possible

[Jessica Paquette via Phabricator via llvm-commits]

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