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

___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

jcai19 closed this revision. jcai19 added a comment. Upsteamed to r369173. 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-comm

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

jcai19 updated this revision to Diff 215710. jcai19 added a comment. Fix frontend mcount unit tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65019/new/ https://reviews.llvm.org/D65019 Files: clang/lib/Basic/Targets/ARM.cpp clang/test/Fro

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

jcai19 added a comment. The changes have been upstreamed to r369147. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65019/new/ https://reviews.llvm.org/D65019 ___ cfe-commits mailing list cfe-commits@lists.llvm.org htt

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL369147: [ARM] push LR before __gnu_mcount_nc (authored by jcai19, committed by ). Changed prior to commit: https://reviews.llvm.org/D65019?vs=214945&id=215664#toc Repository: rL LLVM CHANGES SINCE L

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

jcai19 added a comment. In D65019#1630354 , @nickdesaulniers wrote: > Great! LGTM and thank you for this patch. Please give 24hrs for > @eli.friedman or @kristof.beyls to leave comments before merging. Sounds good! Thanks for all the comments. Repos

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

nickdesaulniers accepted this revision. nickdesaulniers added a subscriber: eli.friedman. nickdesaulniers added a comment. This revision is now accepted and ready to land. Great! LGTM and thank you for this patch. Please give 24hrs for @eli.friedman or @kristof.beyls to leave comments before mer

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

jcai19 marked an inline comment as done. jcai19 added inline comments. Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1922 + const bool Thumb = Opcode == ARM::tBL_PUSHLR; + MachineOperand ReturnAddr = MI.getOperand(0); + assert(ReturnAddr.getReg() == ARM:

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

jcai19 updated this revision to Diff 214945. jcai19 marked an inline comment as done. jcai19 added a comment. Updates based on comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65019/new/ https://reviews.llvm.org/D65019 Files: clang/lib/Ba

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

nickdesaulniers added inline comments. Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1922 + const bool Thumb = Opcode == ARM::tBL_PUSHLR; + MachineOperand ReturnAddr = MI.getOperand(0); + assert(ReturnAddr.getReg() == ARM::LR && "expect LR register!"); -

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

jcai19 marked 2 inline comments as done. jcai19 added inline comments. Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1931 + +// bl__gnu_mcount_nc +MIB = BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(ARM::tBL)); nickdesaulniers wrote: >

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

jcai19 updated this revision to Diff 214908. jcai19 added a comment. Fix a typo. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65019/new/ https://reviews.llvm.org/D65019 Files: clang/lib/Basic/Targets/ARM.cpp llvm/include/llvm/IR/IntrinsicsARM

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

nickdesaulniers added inline comments. Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1931 + +// bl__gnu_mcount_nc +MIB = BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(ARM::tBL)); should there be a space in this comment (and the one on l

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

jcai19 updated this revision to Diff 214889. jcai19 added a comment. clang-format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65019/new/ https://reviews.llvm.org/D65019 Files: clang/lib/Basic/Targets/ARM.cpp llvm/include/llvm/IR/IntrinsicsAR

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

jcai19 marked 4 inline comments as done. jcai19 added inline comments. Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1927 +.add(predOps(ARMCC::AL)) +.addReg(ARM::LR); + efriedma wrote: > I think you need to ensure that lr actual

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

jcai19 updated this revision to Diff 214884. jcai19 added a comment. Mark LR as live-in at (t)BL_PUSHLR instruction. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65019/new/ https://reviews.llvm.org/D65019 Files: clang/lib/Basic/Targets/ARM.cpp

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

efriedma added inline comments. Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1927 +.add(predOps(ARMCC::AL)) +.addReg(ARM::LR); + I think you need to ensure that lr actually contains the correct value, somehow. Normally the ca

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

nickdesaulniers added inline comments. Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:3485 + cast( + Op.getOperand(Op.getOperand(0).getValueType() == MVT::Other ? 1 : 0)) + ->getZExtValue(); `Op.getOperand(0).getValueType() == MVT::Oth

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

jcai19 updated this revision to Diff 214698. jcai19 added a comment. Added back an accidently-deleted blank line. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65019/new/ https://reviews.llvm.org/D65019 Files: clang/lib/Basic/Targets/ARM.cpp l

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

jcai19 added a comment. In D65019#1625780 , @efriedma wrote: > > It seems global-isel does not fall back to DAGISel? > > It does, for targets where it's enabled by default, or if you use the right > flags. I think you want `-global-isel -global-isel-abor

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

jcai19 updated this revision to Diff 214696. jcai19 added a comment. Add proper instruction selection options to unit test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65019/new/ https://reviews.llvm.org/D65019 Files: clang/lib/Basic/Targets/A

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

efriedma added a comment. > It seems global-isel does not fall back to DAGISel? It does, for targets where it's enabled by default, or if you use the right flags. I think you want `-global-isel -global-isel-abort=2`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://r

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

jcai19 marked an inline comment as done. jcai19 added inline comments. Comment at: llvm/test/CodeGen/ARM/gnu_mcount_nc.ll:1-6 +; RUN: llc -mtriple=armv7a-linux-gnueabihf %s -o - | FileCheck %s --check-prefix=CHECK-ARM +; RUN: llc -mtriple=armv7a-linux-gnueabihf %s -o - | FileChe

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

kristof.beyls added inline comments. Comment at: llvm/test/CodeGen/ARM/gnu_mcount_nc.ll:1-6 +; RUN: llc -mtriple=armv7a-linux-gnueabihf %s -o - | FileCheck %s --check-prefix=CHECK-ARM +; RUN: llc -mtriple=armv7a-linux-gnueabihf %s -o - | FileCheck %s --check-prefix=CHECK-ARM-FA

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

jcai19 updated this revision to Diff 214496. jcai19 marked 2 inline comments as done. jcai19 added a comment. clang-format the patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65019/new/ https://reviews.llvm.org/D65019 Files: clang/lib/Basic

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

jcai19 marked 4 inline comments as done. jcai19 added a comment. @efriedma I have changed my implementation to lower llvm.gnu.eabi.mcount intrinsic into pseudo instructions directly, instead of first lowering them into SelectionDAG call nodes. Thanks. Comment at: llvm/lib/Tar

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

jcai19 updated this revision to Diff 214491. jcai19 added a comment. Lower the intrinsic to pseudo instructions directly, instead of SelectDAG nodes first. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65019/new/ https://reviews.llvm.org/D65019 F

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

kristof.beyls added a comment. I've just added a few fly-by nits; I'm afraid I didn't do an in-depth review. Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1156 MachineInstr &MI = *MBBI; + LLVM_DEBUG(dbgs() << "ARMExpandPseudo::ExpandMI: " << MI << "\n"); unsig

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

jcai19 marked an inline comment as done. jcai19 added inline comments. Comment at: clang/lib/Basic/Targets/ARM.cpp:325 + ? "llvm.arm.gnu.eabi.mcount" : "\01mcount"; nickdesaulniers wrote: > Doesn't require c

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

jcai19 updated this revision to Diff 214263. jcai19 added a comment. Lower the new intrinsic when legalizing DAGs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65019/new/ https://reviews.llvm.org/D65019 Files: clang/lib/Basic/Targets/ARM.cpp

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

jcai19 added a comment. In D65019#1621670 , @efriedma wrote: > Yes, it's technically a "call", but you don't need the call lowering code. > That's dedicated to stuff like passing arguments, returning values, checking > whether the function can be tail-c

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

efriedma added a comment. Yes, it's technically a "call", but you don't need the call lowering code. That's dedicated to stuff like passing arguments, returning values, checking whether the function can be tail-called, etc. None of that applies here; the intrinsic always corresponds to exactl

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

jcai19 marked an inline comment as done. jcai19 added a comment. In D65019#1619511 , @efriedma wrote: > This seems better. > > I'm not sure I follow why this needs special handling in > SelectionDAGBuilder::visitIntrinsicCall, as opposed to just using >

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

nickdesaulniers marked an inline comment as done. nickdesaulniers added inline comments. Comment at: clang/lib/Basic/Targets/ARM.cpp:325 + ? "llvm.arm.gnu.eabi.mcount" : "\01mcount"; Doesn't require changes,

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

efriedma added a comment. This seems better. I'm not sure I follow why this needs special handling in SelectionDAGBuilder::visitIntrinsicCall, as opposed to just using ISD::INTRINSIC_VOID like other similar target-specific intrinsics. (You can custom-lower INTRINSIC_VOID in ARMTargetLowering:

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

jcai19 marked 4 inline comments as done. jcai19 added inline comments. 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(

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

jcai19 updated this revision to Diff 213762. jcai19 added a comment. Update based on comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65019/new/ https://reviews.llvm.org/D65019 Files: clang/lib/Basic/Targets/ARM.cpp llvm/include/llvm/IR

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

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 ARMLowerPI

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

jcai19 marked an inline comment as done. jcai19 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 =

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

jcai19 updated this revision to Diff 213748. jcai19 marked 6 inline comments as done. jcai19 added a comment. Update based on comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65019/new/ https://reviews.llvm.org/D65019 Files: clang/lib/Bas

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

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 + + // Re

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

jcai19 added a comment. In D65019#1615898 , @efriedma wrote: > > introduce quite some code to the target-independent part of SelectionDAG, > > specifically SelectionDAGBuilder and SelectionDAG classes > > Not sure why you think this would be necessary. Ev

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

jcai19 updated this revision to Diff 213728. jcai19 added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Introduce a new ARM intrinsic for __gnu_mcount_nc. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65019/new/