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

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

Some comments inline.



================
Comment at: lib/CodeGen/MachineOutliner.cpp:883
 
+    size_t SequenceOverhead = StringLen;
+
----------------
I wonder why this (and other variables) are `size_t` and not just unsigned?


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:4461-4469
+  for (auto I = MBB.rbegin(); I != MBB.rend(); LRU.stepBackward(*I), ++I) {
+    // Is the link register dead at this point?
+    if (!LRU.available(AArch64::LR))
+      return false;
+  }
+
+  // The link register is dead throughout the entire block. We can skip saving
----------------
This one seems a decent candidate for `any_of`, but the use of `stepBackward()` could make it more difficult.


================
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:
----------------
Is your cost model for the call overhead documented somewhere? If not, can you add a comment here?


================
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);
----------------
Have you considered rewriting this code in terms of `llvm::any_of` (or `all_of`) for clarity?


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:4538-4541
 bool AArch64InstrInfo::isFunctionSafeToOutlineFrom(MachineFunction &MF) const {
-  return MF.getFunction()->hasFnAttribute(Attribute::NoRedZone);
+  return MF.getFunction()->hasFnAttribute(Attribute::NoRedZone) &&
+         !MF.getFunction()->hasAddressTaken();
 }
----------------
Can you please add a comment here? It's not obvious when it's safe to outline without understanding the whole picture.


https://reviews.llvm.org/D36721