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

[Yvan Roux via Phabricator via llvm-commits]

yroux marked 5 inline comments as done.
yroux added a comment.

Thanks for the review Jessica,

I'll split it into two part and add test cases



================
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(); }))
----------------
paquette wrote:
> This should have a testcase?
OK


================
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;
----------------
paquette wrote:
> I wonder if this check should move to the generic outliner code, since it seems like every target requires it.
hmm, yes maybe looking at it more closely for ARM it is in fact __gnu_mcount_nc and __mcount but _mcount is everywhere else and I think it only makes sense if we are in linux context (not needed for bare metal) 


================
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;
----------------
paquette wrote:
> There should be a testcase for outlining calls?
ok


================
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);
----------------
paquette wrote:
> There should be a testcase showing the save/restore from SP.
It is tested in machine-outliner-nosave-and-regs.mir but maybe needs to put inside a dedicated testcase


================
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
----------------
paquette wrote:
> 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.
yes it is, but you are right I'll explicitly checked the output 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76069