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

[Tim Northover via Phabricator via llvm-commits]

t.p.northover added a comment.

Thanks very much for working on this, those numbers look promising. I had a quick read over, and have a few comments. I haven't looked at the checking logic in detail yet, mostly just the CodeGen part.



================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5298
+  unsigned FrameID = MachineOutlinerDefault;
+  unsigned NumBytesToCreateFrame = Thumb ? 2 : 4;
+
----------------
I think we should probably try to do something about these bare numbers as costs. Especially as things are pretty complicated with ARM/Thumb2/Thumb1. It's sometimes not quite clear just what cost we're talking about.

Maybe a special struct type with named costs that can be referred to when needed?


    struct OutlineCosts {
      int TailCall;
      int Call;
      int SaveLRToStack;
      [...]
    };


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5722
+    if (Thumb)
+      TC = BuildMI(MF, DebugLoc(), get(ARM::tTAILJMPdND))
+               .add(Call->getOperand(2))
----------------
I think it's more idiomatic to use BuildMI with an MBB and an iterator rather than separate build & insert steps.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5755
+
+    // Insert a save before the outlined region
+    MachineInstr *Save;
----------------
Comment doesn't quite fit here, and code should maybe be factored into a helper function.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5893
+    if (Thumb) {
+      Save = BuildMI(MF, DebugLoc(), get(ARM::tMOVr), Reg)
+                 .addReg(ARM::LR)
----------------
This probably ought to be a copyPhysReg, if you look in Thumb1InstrInfo are some really weird edge cases on old CPUs. As a bonus you don't even need to care about ARM vs Thumb if you do that.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5921-5923
+        Save = BuildMI(MF, DebugLoc(), get(ARM::tPUSH))
+                   .add(predOps(ARMCC::AL))
+                   .addReg(ARM::LR);
----------------
I think we need to talk about stack alignment here. This sequence misaligns the stack on ARM and Thumb2.

If you take the view that the call to `OUTLINED_FUNCTION` is not a public interface (plausible, but by no means a definite) then the corresponding `push` for a nested call in `buildOutlinedFrame` will realign the stack except on Apple's watchOS ABI (armv7k, which has a 16-byte requirement).

If it is public, then this definitely has to change.

Either way, it should definitely be noted in comments.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5966
+      && Subtarget.isThumb1Only()) {
+  It++;
+  It = MBB.insert(It, BuildMI(MF, DebugLoc(), get(ARM::tMOVr))
----------------
Indentation.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5967-5970
+  It = MBB.insert(It, BuildMI(MF, DebugLoc(), get(ARM::tMOVr))
+                  .addReg(ARM::LR, RegState::Define)
+                  .addReg(ARM::R7, RegState::Kill)
+                  .add(predOps(ARMCC::AL)));
----------------
Haven't we just popped what originally came from r7? And If there's no frame pointer couldn't r7 be a normal register that has changed inside the outlined function?

Also, I think BuildMIs that insert at the same time would definitely be clearer in this function. In outline:

    // Do all kinds of save
    if (Thumb)
       BuildMI(...);
    else
       BuildMI(...);

    // Insert call
   BuildMI(....);

    // Do all kinds of restore
    if (Thumb)
      BuildMI(...);
    else
      BuildMI(...);

I'd be pretty tempted to add virtual helper functions actually: `spillLRToSTack` and `restoreLRFromStack`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57054