[PATCH] D76069: [ARM][MachineOutliner] Add default mode.

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

I think that there's a lot to be tested here which isn't covered yet. Maybe it would make sense to split this patch a bit further? E.g. factor out outlining calls into a separate patch?



================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5763-5780
+  // Does every candidate's MBB contain a call?  If so, then we might have a
+  // call in the range.
+  if (FlagsSetInAll & MachineOutlinerMBBFlags::HasCalls) {
+    // Check if the range contains a call. These require a save + restore of the
+    // link register.
+    if (std::any_of(FirstCand.front(), FirstCand.back(),
+                    [](const MachineInstr &MI) { return MI.isCall(); }))
----------------
This should have a testcase?


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5925-5928
+    // Never outline calls to mcount.  There isn't any rule that would require
+    // this, but the Linux kernel's "ftrace" feature depends on it.
+    if (Callee && Callee->getName() == "\01_mcount")
+      return outliner::InstrType::Illegal;
----------------
I wonder if this check should move to the generic outliner code, since it seems like every target requires it.


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5958-5960
+    // At this point, we can say that CalleeMF ought to not pass anything on the
+    // stack. Therefore, we can outline it.
+    return outliner::InstrType::Legal;
----------------
There should be a testcase for outlining calls?


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:6170-6171
 
-  It = MBB.insert(It, CallMIB);
-  return It;
+  // We have the default case. Save and restore from SP.
+  saveLROnStack(MBB, It);
+  CallPt = MBB.insert(It, CallMIB);
----------------
There should be a testcase showing the save/restore from SP.


================
Comment at: llvm/test/CodeGen/ARM/machine-outliner-nosave-and-regs.mir:67-83
+name:           outline_save_noreg
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    ; CHECK-LABEL: bb.0:
+    ; CHECK: OUTLINED
+    liveins: $lr, $r0, $r1, $r2, $r3, $r4, $r5, $r6, $r7, $r8, $r8, $r9, $r10, $r11
----------------
Does this testcase save + restore LR? It would be good to check for the save + restore (or lack thereof) explicitly instead of just checking for the outlined call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76069