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

[Yvan Roux via Phabricator via llvm-commits]

yroux added a comment.

Hi Sam,

Thanks for your comments.



================
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
----------------
samparker wrote:
> 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.
Good question, I'll test that


================
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;
----------------
samparker wrote:
> There's the getPredicate helper is ARMBaseInstrInfo which looks like it would be useful here.
Ah yes, and there is even isPredicated which might simplify the logic 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))
----------------
samparker wrote:
> Probably worth doing these simple checks earlier. Should we also be concerned about the PC too?
Well, the phasing is important here, if we move the checking of LR or SP usage earlier it will make the outlining of calls illegal since they implicitly modifies these registers or we can go back to my previous version which was checking the explicit usage but it introduces redundancy.

hmm.. maybe we can be conservative with PC, it is not verified in AArch64 code @paquette do you think it can be an issue?


================
Comment at: llvm/lib/Target/ARM/ARMTargetMachine.cpp:553
   addPass(createARMConstantIslandPass());
-  addPass(createARMLowOverheadLoopsPass());
+  if (!MachineOutlinerEnabled)
+    addPass(createARMLowOverheadLoopsPass());
----------------
samparker wrote:
> 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.
What do you mean by "for correctness" ?

I think that it makes more sense that until MachineOutliner and LowOverheadLoops can work together, we have loloops enabled on targets which have LOB support unless it is explicitly disabled by -disable-arm-loloops flag or if the user wants machine outlining with the flag -moutline.   If we do that in the opposite way it means that passing the flag -moutline will have no impact on such targets unless the -disable-arm-loloops flag is used



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76066