[PATCH] D76066: [ARM][MachineOutliner] Add Machine Outliner support for ARM

[Sam Parker via Phabricator via cfe-commits]

samparker added a comment.

Hi Yvan,

Thanks for adding the tests, I've added a few concerns and questions.



================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5606
+  // candidates.
+  auto CantGuaranteeValueAcrossCall = [&TRI](outliner::Candidate &C) {
+    // If the unsafe registers in this block are all dead, then we don't need
----------------
Does this code work with the DSP instructions that read/write the Q and GE flags? I have a nasty feeling that we don't model their register usage.


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5765
+    if (Opc == ARM::BX_RET || Opc == ARM::tBX_RET || Opc == ARM::MOVPCLR) {
+      if (MI.getOperand(0).getImm() != ARMCC::AL)
+        return outliner::InstrType::Illegal;
----------------
There's the getPredicate helper is ARMBaseInstrInfo which looks like it would be useful here.


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5801
+
+  // Don't touch the link register
+  if (MI.readsRegister(ARM::LR, TRI) || MI.modifiesRegister(ARM::LR, TRI))
----------------
Probably worth doing these simple checks earlier. Should we also be concerned about the PC too?


================
Comment at: llvm/lib/Target/ARM/ARMTargetMachine.cpp:553
   addPass(createARMConstantIslandPass());
-  addPass(createARMLowOverheadLoopsPass());
+  if (!MachineOutlinerEnabled)
+    addPass(createARMLowOverheadLoopsPass());
----------------
We'll need the LowOverheadLoops pass to run for correctness, so we should instead only add the MachineOutliner if the subtarget doesn't support LOB.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76066