[PATCH] D57054: [MachineOutliner][ARM][RFC] Add Machine Outliner support for ARM

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a reviewer: efriedma.
efriedma added inline comments.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5127
+///
+/// * Call construction overhead: 3 (save + BL + restore)
+/// * Frame construction overhead: 1 (BX)
----------------
You probably want to describe the overhead in bytes, not instructions, since they're substantially different in Thumb mode.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5302
+  // tail call all of the candidates.
+  if (Subtarget.supportsTailCall()
+      && RepeatedSequenceLocs[0].back()->isTerminator()) {
----------------
supportsTailCall is more general than you need for MachineOutlinerTailCall; on Thumb1, you can outline a tail call if LR isn't live-in (if the terminator is a tPOP_RET etc.).


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5310
+  else if (Subtarget.supportsTailCall()
+           && (LastInstrOpcode == ARM::BL || LastInstrOpcode == ARM::BLX
+               || LastInstrOpcode == ARM::tBL)) {
----------------
supportsTailCall is a bit more general than you need for MachineOutlinerThunk; rewriting tBLX to tBX is legal in Thumb1.  Also, missing check for tBLX (both here and elsewhere).


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5528
+  case ARM::t2IT:
+  case ARM::tPICADD:
+  case ARM::PICADD:
----------------
This list isn't complete for Thumb; missing tLDRpci_pic, t2LDRpci_pic, t2MOVi16_ga_pcrel, t2MOVTi16_ga_pcrel.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5722
+    if (Thumb)
+      TC = BuildMI(MF, DebugLoc(), get(ARM::tTAILJMPdND))
+               .add(Call->getOperand(2))
----------------
t.p.northover wrote:
> I think it's more idiomatic to use BuildMI with an MBB and an iterator rather than separate build & insert steps.
There are two different Thumb direct tail-call opcodes: tTAILJMPdND and tTAILJMPd; the latter is used for MachO targets because the assembler doesn't support relaxing a Thumb1 jump instruction.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5828
+  //current feature set.
+  if (!Subtarget.isThumb1Only()) {
+    MachineInstr *ret = BuildMI(MF, DebugLoc(),
----------------
Not sure why you aren't using tPOP_RET for Thumb2.


================
Comment at: lib/Target/ARM/ARMTargetMachine.cpp:515
+void ARMPassConfig::addPreEmitPass2() {
   addPass(createARMConstantIslandPass());
 }
----------------
paquette wrote:
> Is this always necessary, or only when the outliner is added? If it's not always necessary, would it be possible to only add it if the outliner was added?
createARMConstantIslandPass always has to run at some point.  Running it after the outliner allows outlining constant pool references (which probably helps codesize, although it could increase codesize in some cases).

Looking at the passes that run between addPreEmitPass and addPreEmitPass2, moving it makes sense.  I think the only other affected pass is FuncletLayout, and it probably wants to run before ConstantIslands anyway (ARM Windows EH currently isn't implemented, but might be in the future).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57054