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

[Yvan Roux via Phabricator via llvm-commits]

yroux added a comment.

In D76066#2014697 <https://reviews.llvm.org/D76066#2014697>, @samparker wrote:

> Thanks for adding the MVE changes, but I also still don't see any DSP tests, i.e QADD, SADD16.


hmm... I was wrong w/r to DSP instructions Q and GE flags handling, I don't get what I did in my tests, but there is no info about CPSR in the MIR attached to these instructions.  So, you had a good feeling and I can disable them, but as Eli said in his comments we don't really need to worry about APCS but only to linker veneers and I don't think a linker would clear the sticky Q bit or touch the GE ones.

Do you think we can let it as it is ?



================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5607
+  // * Register R12(IP),
+  // * Condition codes (and thus the CPSR register)
+  //
----------------
efriedma wrote:
> If you control all the instructions that execute, you don't need to worry about what the procedure call standard says.  You do need to worry about linker veneers if the outlined function is in a different section, though.
> 
> So you need to worry about `R12`/`CPSR` on entry to the outlined function, but not on exit from the outlined function.
Yes I agree and it is what is done here, I put the same comment as what was done on AArch64 to explain where it comes from, but as it is mentioned below we remove the candidate when these registers are live into or across, which means that we can outline cases where def and use are inside the outlined region or the def inside and the use after the exit as you said.

I've modify the machine-outliner-unsafe-registers.mir test in the update version I'll submit to exhibit this.


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5753
+  // ahead and skip over them.
+  if (MI.isKill())
+    return outliner::InstrType::Invisible;
----------------
samparker wrote:
> Should IMPLICIT_DEF instructions be considered invisible too?
Yes, I'll add it


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5813
+  // Don't touch the link register
+  if (MI.readsRegister(ARM::LR, TRI) || MI.modifiesRegister(ARM::LR, TRI))
+    return outliner::InstrType::Illegal;
----------------
efriedma wrote:
> Why do you need to forbid outlining code that touches LR or SP? None of the new instructions you're generating read or clobber them.   (It might start mattering if you add support for additional forms of outlining, or Thumb1 support, but this patch has neither.)
I agree that there is no issue with SP until we modify it, I'll move that to the right patch, for LR it is needed here, when doing thunk outlining the call to the outlined function is a BL and if an instruction uses LR in the outlined region we will have a problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76066