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

Thu Jan 24 01:58:16 PST 2019

t.p.northover added inline comments.

================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5921-5923
+        Save = BuildMI(MF, DebugLoc(), get(ARM::tPUSH))
+                   .add(predOps(ARMCC::AL))
+                   .addReg(ARM::LR);
----------------
yroux wrote:
> t.p.northover wrote:
> > 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.
> Correct me Jessica if I'm wrong, my understanding of outlined functions is that they are not public interfaces and don't even respect the ABI but it is not an issue since we have the control on all the call sites, but I didn't thought of such cases of stack misalignment and targets requirement.  I need to think more about it, but I agree at least a comment is needed.
That's a good point about the PCS, so it's almost certainly not morally a public interface.

We sort of still have to worry about what the linker might insert between the branch to the outlined function and actually getting there, but since this is all within a single linkage unit I think the worst would be a simple branch island, unlikely to rely on stack alignment. And if it did, it might rely on far more that we break.

We do still need to guarantee 16-byte alignment for nested calls on armv7k though.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57054