[PATCH] D65019: [ARM] push LR before __gnu_mcount_nc on ARM

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

Thanks so much for this patch! I look forward to support for `__gnu_mcount` for the arm32 Linux kernel.

> What a horrible function. AAPCS? Who cares about that?

haha



================
Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1944
+
+      // Replace the pseudo instruction with a call instruction
+      MachineInstrBuilder MIB = BuildMI(MBB, MBBI, MI.getDebugLoc(),
----------------
Here down seems to match the previous case? Maybe you could "Dont Repeat Yourself (DRY)" up the code by creating a shared function?


================
Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1950
+        MIB.add(MO);
+      }
+      MI.eraseFromParent();
----------------
No need for `{}` for single statement blocks.


================
Comment at: llvm/lib/Target/ARM/ARMFastISel.cpp:211
     unsigned ARMMoveToIntReg(MVT VT, unsigned SrcReg);
-    unsigned ARMSelectCallOp(bool UseReg);
+    unsigned ARMSelectCallOp(bool UseReg, bool PushLR = false);
     unsigned ARMLowerPICELF(const GlobalValue *GV, unsigned Align, MVT VT);
----------------
How many other call sites would have to be updated if this was not a default parameter?


================
Comment at: llvm/lib/Target/ARM/ARMFastISel.cpp:2417
   // Issue the call.
-  unsigned CallOpc = ARMSelectCallOp(UseReg);
+  unsigned CallOpc = ARMSelectCallOp(UseReg, Callee && Callee->getName() == "\01__gnu_mcount_nc");
   MachineInstrBuilder MIB = BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt,
----------------
efriedma wrote:
> 80 cols
also, what's up with `\01`?


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:2298
+    }
+  }
+
----------------
Ditto on single statement bodies. `{}`


================
Comment at: llvm/test/CodeGen/ARM/gnu_mcount_nc.ll:5
+define dso_local i32 @foo(i64) local_unnamed_addr #0 {
+; CHECK-ARM:        stmdb   sp!, {lr}
+; CHECK-ARM-NOT:    stmdb   sp!, {lr}
----------------
Don't we want to check that these occur in a parent function before a call to a child function?


================
Comment at: llvm/test/CodeGen/ARM/gnu_mcount_nc.ll:6
+; CHECK-ARM:        stmdb   sp!, {lr}
+; CHECK-ARM-NOT:    stmdb   sp!, {lr}
+; CHECK-ARM-NEXT:   bl      __gnu_mcount_nc
----------------
why does the `-NOT` case duplicate the non-`NOT` case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65019