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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.


================
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);
----------------
jcai19 wrote:
> nickdesaulniers wrote:
> > How many other call sites would have to be updated if this was not a default parameter?
> There should be only 2 references to this function. But this argument is likely to be false in most cases.
Then I'd just make it an explicit arg and update the 2 call sites.  If there were many call sites, then the default param would cut down on code churn, but I don't think 2 call sites is unreasonable to just be explicit about such arguments.


================
Comment at: llvm/lib/Target/ARM/ARMFastISel.cpp:2418
+  bool PushLR = IntrinsicName &&
+      !memcmp(IntrinsicName, "\01__gnu_mcount_nc", sizeof("\01__gnu_mcount_nc"));
+  unsigned CallOpc = ARMSelectCallOp(UseReg, PushLR);
----------------
`memcmp` is a code smell in a C++ codebase, but I see that `IntrinsicName` is a C style string.  Is there a reason why `strncmp` isn't used?


================
Comment at: llvm/lib/Target/ARM/ARMFastISel.cpp:2587
   }
+  case Intrinsic::arm_gnu_eabi_mcount: {
+    return SelectCall(&I, "\01__gnu_mcount_nc");
----------------
`{}` are not needed here since you're not introducing a new scope for variables.


================
Comment at: llvm/test/CodeGen/ARM/gnu_mcount_nc.ll:11
+entry:
+  %cmp = icmp slt i32 %n, 2
+  br i1 %cmp, label %return, label %if.end
----------------
This test case can probably be simplified to just a call and ret void.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65019