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

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

Thanks for looking into this!

I added some comments.

I think that the biggest thing that stands out to me is that supporting Thumb means that some of the target hooks get extremely long. It'd be nice to split those up into two cases if possible.



================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5428
+bool ARMBaseInstrInfo::checkAndUpdateStackOffset(MachineInstr *MI,
+                                                 int64_t fixup,
+                                                 bool Updt) const {
----------------
Capitalization


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5704-5706
+    if (!MI.mayLoadOrStore()) {
+      continue;
+    }
----------------
Don't need brackets here.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5844-5846
+MachineBasicBlock::iterator ARMBaseInstrInfo::insertOutlinedCall(
+    Module &M, MachineBasicBlock &MBB, MachineBasicBlock::iterator &It,
+    MachineFunction &MF, const outliner::Candidate &C) const {
----------------
This function is pretty long. I think it'd be a bit easier to follow if it was split into the Thumb and non-Thumb cases if possible.

Maybe something like a `insertOutliner[WhateverInstructionWe'reAdding]Inst` which returns the next value of `It` would work? E.g

```
if (C.CallConstructionID == MachineOutlinerTailCall) {
    It = insertOutlinerTailCallInst(Thumb);
    return It;
}

if (C.CallConstructionID == MachineOutlinerNoLRSave ||
      C.CallConstructionID == MachineOutlinerThunk) {
    It  = insertOutlinerCallInst(Thumb);
    return It;
}

...

```


================
Comment at: lib/Target/ARM/ARMTargetMachine.cpp:242
+  setMachineOutliner(true);
+  setSupportsDefaultOutlining(true);
 }
----------------
Hmmm, this is somewhat bold. I (conservatively) think that it'd be good to split turning it on by default into a separate patch.


================
Comment at: lib/Target/ARM/ARMTargetMachine.cpp:515
+void ARMPassConfig::addPreEmitPass2() {
   addPass(createARMConstantIslandPass());
 }
----------------
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?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57054