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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits