[ARM] push LR before __gnu_mcount_nc

[jcai19]

Summary

__gnu_mcount_nc expects the value of LR register to be the top value of the stack on ARM32.
Clang currently does not push the value before the call.

Fixes PR33845.

[jcai19]

Update the commit message.

[george.burgess.iv]

Thanks for this!

I realize I'm not a reviewer, but have a few nits in passing anyway. :)

I don't plan to stamp this because I've zero familiarity here.

llvm/lib/Target/ARM/CustomCallLoweringPass.cpp

21 ↗

(On Diff #210904)

nit: when overriding a virtual function,

bool runOnMachineFunction(MachineFunction &MF) override {

is preferred over

virtual bool runOnMachineFunction(MachineFunction &MF) {

25 ↗

(On Diff #210904)

nit: can this be iterated over via a range-for loop?

for (MachineBasicBlock &MBB : MF) {
  for (MachineInstruction &MI : MBB) {
    // ...
  }
}

29 ↗

(On Diff #210904)

nit: similar range-for question over something like BBI->operands()?

35 ↗

(On Diff #210904)

please clang-format all patches

[efriedma]

I don't like introducing magic calling convention rules for a non-intrinsic function with a specific name; we should model this more explicitly somehow. Probably the simplest thing to do would be to introduce an ARM intrinsic, lower it to a pseudo-instruction, and expand it late (in ExpandPostRAPseudos or something like that).

[jcai19]

Add additional test on GV and update format.

[jcai19]

In D65019#1594289, @efriedma wrote:

I don't like introducing magic calling convention rules for a non-intrinsic function with a specific name; we should model this more explicitly somehow. Probably the simplest thing to do would be to introduce an ARM intrinsic, lower it to a pseudo-instruction, and expand it late (in ExpandPostRAPseudos or something like that).

Thanks for the suggestions, will start work on it.. I also realized this solution may have other issues that @t.p.northover kindly pointed out in another code review that I accidentally creates (https://reviews.llvm.org/D65037) for the same change.

[jcai19]

Introduce new ARMISD node for __gnu_mcount_nc.

[jcai19]

Remove an unnecssary blank line.

[jcai19]

In D65019#1594704, @jcai19 wrote:

In D65019#1594289, @efriedma wrote:

I don't like introducing magic calling convention rules for a non-intrinsic function with a specific name; we should model this more explicitly somehow. Probably the simplest thing to do would be to introduce an ARM intrinsic, lower it to a pseudo-instruction, and expand it late (in ExpandPostRAPseudos or something like that).

So I have implemented the codegen part as suggested. To add a new intrinsic for this call seems would introduce quite some code to the target-independent part of SelectionDAG, specifically SelectionDAGBuilder and SelectionDAG classes, just for ARM. Is that wanted? Thanks.

[efriedma]

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. Every target has target-specific intrinsics, and we have infrastructure to ensure they don't require any changes to target-independent code.

llvm/lib/Target/ARM/ARMFastISel.cpp

2417 ↗

(On Diff #213159)

80 cols

[jcai19]

Introduce a new ARM intrinsic for __gnu_mcount_nc.

[jcai19]

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. Every target has target-specific intrinsics, and we have infrastructure to ensure they don't require any changes to target-independent code.

I guess what I was trying to say was that I could not find a way to handle the new ARM intrinsic in SelectionDAGBuilder::visitTargetIntrinsic easily with the existing code so I had to introduce a case just for the ARM intrinsic in SelectionDAGBuilder::visitIntrinsic, and I am not sure if that is acceptable.

[nickdesaulniers]

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

llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp

1944

Here down seems to match the previous case? Maybe you could "Dont Repeat Yourself (DRY)" up the code by creating a shared function?

1950

No need for {} for single statement blocks.

llvm/lib/Target/ARM/ARMFastISel.cpp

211 ↗

(On Diff #213159)

How many other call sites would have to be updated if this was not a default parameter?

2417 ↗

(On Diff #213159)

also, what's up with \01?

llvm/lib/Target/ARM/ARMISelLowering.cpp

2365

Ditto on single statement bodies. {}

llvm/test/CodeGen/ARM/gnu_mcount_nc.ll

6

Don't we want to check that these occur in a parent function before a call to a child function?

7

why does the -NOT case duplicate the non-NOT case?

[jcai19]

Update based on comments.

[jcai19]

llvm/lib/Target/ARM/ARMFastISel.cpp

211 ↗

(On Diff #213159)

There should be only 2 references to this function. But this argument is likely to be false in most cases.

2417 ↗

(On Diff #213159)

I am not sure why the name is prefixed with \01 either but it was there in the original code.

llvm/test/CodeGen/ARM/gnu_mcount_nc.ll

7

They are unnecessary, I have removed them.

[nickdesaulniers]

llvm/lib/Target/ARM/ARMFastISel.cpp

2418 ↗

(On Diff #213748)

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?

2587 ↗

(On Diff #213748)

{} are not needed here since you're not introducing a new scope for variables.

211 ↗

(On Diff #213159)

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.

llvm/test/CodeGen/ARM/gnu_mcount_nc.ll

12

This test case can probably be simplified to just a call and ret void.

[jcai19]

Update based on comments.

[jcai19]

llvm/lib/Target/ARM/ARMFastISel.cpp

2418 ↗

(On Diff #213748)

Good catch! I was thinking about one of str* functions and somehow ended up using memcpy. Guess I haven't written C code for a while :). Anyway, maybe strcmp is better here as the size of the two strings should match too?

[efriedma]

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::LowerOperation, if that makes it easier.)

I'd just skip changing fast-isel; with an intrinsic, if fast-isel misses, we just fall back to the SelectionDAG code that does the right thing.

[nickdesaulniers]

clang/lib/Basic/Targets/ARM.cpp

326

Doesn't require changes, but for anyone curious about the \01, see the comment in MangleContext::mangleName.

[jcai19]

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 ISD::INTRINSIC_VOID like other similar target-specific intrinsics. (You can custom-lower INTRINSIC_VOID in ARMTargetLowering::LowerOperation, if that makes it easier.)

Thanks for the suggestion, and I agree the code would look cleaner this way. But I have some questions on implementation details, and please bear with me if they seem naive since I am new to backend. So I have been trying to reuse the code of ARMTargetLowering::LowerCall to build a SelectionDAG call node for the new intrinsic, which essentially is a function call with a push instruction before. This is also how some intrinsics like memcpy or memet implemented, which get lowered to target-specific calls at SelectionDAGBuilder::visitIntrinsicCall. If we wait until ARMTargetLowering::LowerOperation when legalizing DAGs to lower the intrinsic, then I am not sure if we can still reuse the code, as some of the information needed is gone by this stage. I did see code handling ARM intrinsic on ARMTargetLowering::LowerOperation, but they didn't seem to need to generate function calls later.

[efriedma]

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 exactly one pseudo-instruction, a BL_PUSHLR.

[jcai19]

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-called, etc. None of that applies here; the intrinsic always corresponds to exactly one pseudo-instruction, a BL_PUSHLR.

Thanks for the clarification. I will look into it.

[jcai19]

Lower the new intrinsic when legalizing DAGs.

[jcai19]

clang/lib/Basic/Targets/ARM.cpp

326

Thanks for the reference!

[kristof.beyls]

I've just added a few fly-by nits; I'm afraid I didn't do an in-depth review.

llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp

1157

I wonder whether this is a good debug printing line to commit?
IIUC, this will print every MI instruction that gets looked at by ArmExpandPseudo.
I would imagine that that could produce too much noise. It'd be more interesting if only the MIs that actually got transformed would be printed.
But maybe best to just not add this debug printing line in this patch?

1931–1932

Did you clang-format the patch?

llvm/test/CodeGen/ARM/gnu_mcount_nc.ll

2–3

Given that the push-lr transform only gets implemented for DAGISel (IIUC), maybe it'd be useful to also have test run lines that check the correct thing happens when using fastisel and globalisel (presumably by falling back to DAGISel)?

[jcai19]

Lower the intrinsic to pseudo instructions directly, instead of SelectDAG nodes first.

[jcai19]

@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.

llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp

1157

Sorry, I forgot to remove it. I was using it to debug my change locally.

llvm/test/CodeGen/ARM/gnu_mcount_nc.ll

2–3

That's a good point. Checks added for fast-isel and global-isel.

[jcai19]

clang-format the patch.

[kristof.beyls]

llvm/test/CodeGen/ARM/gnu_mcount_nc.ll

2–7

It seems the -fast-isel/-global-isel command line options are missing in the RUN lines aiming to test fast and global isel do the right thing?

[jcai19]

llvm/test/CodeGen/ARM/gnu_mcount_nc.ll

2–7

Sorry must have forgotten to add the instruction selection options while copying the RUN commands. Just tested locally and -fast-isel option worked, although -global-isel failed due to "LLVM ERROR: unable to map instruction: G_INTRINSIC_W_SIDE_EFFECTS intrinsic(@llvm.arm.gnu.eabi.mcount)". It seems global-isel does not fall back to DAGISel? Will have to investigate further.

[efriedma]

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?

[jcai19]

Add proper instruction selection options to unit test.

[jcai19]

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-abort=2?

Yes -global-isel-abort=2 fixed the issue. Thanks!

[jcai19]

Added back an accidently-deleted blank line.

[nickdesaulniers]

llvm/lib/Target/ARM/ARMISelLowering.cpp

3552

Op.getOperand(0).getValueType() == MVT::Other ? 1 : 0 could be replaced with Op.getOperand(0).getValueType() == MVT::Other

3554

Why construct dl if we don't use it in the default case, or under certain conditions below? Maybe move the definition closer to its use below. Though I see temporary SDLoc(Op) below, which should be sufficient (so you can remove dl).

[efriedma]

llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp

1928

I think you need to ensure that lr actually contains the correct value, somehow. Normally the call will come before anything that would clobber lr, but you're not actually enforcing that anywhere: LR isn't listed as an input to BL_PUSHLR.

To make this work correctly, I think the return address actually needs to be an argument to the BL_PUSHLR instruction. See ARMTargetLowering::LowerRETURNADDR for how to make an appropriate copy.

llvm/test/CodeGen/ARM/gnu_mcount_nc.ll

7

Please add -verify-machineinstrs to all these invocations.

[jcai19]

Mark LR as live-in at (t)BL_PUSHLR instruction.

[jcai19]

llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp

1928

My takeaway from your comment is to mark LR explicitly alive to make sure compiler will restore LR if it clobbers the register before the BL_PUSHLR instruction. Did I understand correctly? Thanks. Anyway, this change seems to be needed once I turned on -verify-machineinstrs in the unit test, which complained the push/stmdb instruction trying to use a dead LR register.

[jcai19]

clang-format

[nickdesaulniers]

llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp

1932

should there be a space in this comment (and the one on line 1941) between bl and __gnu_mcount_nc?

[jcai19]

Fix a typo.

[jcai19]

llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp

1932

Yes you are right. Thanks!

[nickdesaulniers]

llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp

1923

This could be Register ReturnReg = MI.getOperand(0).getReg(); then the below cleaned up. DRY

(and a few more opportunities in the return values of ARMTargetLowering::LowerINTRINSIC_VOID)

With that change, LGTM, and thank you for the patch!

[jcai19]

Updates based on comments.

[jcai19]

llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp

1923

Thank you for all the comments! I have made changes accordingly.

[nickdesaulniers]

Great! LGTM and thank you for this patch. Please give 24hrs for @eli.friedman or @kristof.beyls to leave comments before merging.

[jcai19]

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.

[jcai19]

This comment was removed by jcai19.

[jcai19]

Fix frontend mcount unit tests.

[jcai19]

Upsteamed to r369173.

clang/lib/Basic/Targets/ARM.cpp

ARMTargetInfo::ARMTargetInfo(const llvm::Triple &Triple,
   // the alignment of the zero-length bitfield is greater than the member
   // that follows it, `bar', `bar' will be aligned as the  type of the
   // zero length bitfield.
   UseZeroLengthBitfieldAlignment = true;
 
   if (Triple.getOS() == llvm::Triple::Linux ||
       Triple.getOS() == llvm::Triple::UnknownOS)
     this->MCountName = Opts.EABIVersion == llvm::EABI::GNU
-                           ? "\01__gnu_mcount_nc"
+                           ? "llvm.arm.gnu.eabi.mcount"
                            : "\01mcount";
 
   SoftFloatABI = llvm::is_contained(Opts.FeaturesAsWritten, "+soft-float-abi");
 }
 
 StringRef ARMTargetInfo::getABI() const { return ABI; }
 
 bool ARMTargetInfo::setABI(const std::string &Name) {
   ABI = Name;

clang/test/Frontend/gnu-mcount.c

 // CHECK-ARM64-EABI-FREEBSD-NOT: attributes #{{[0-9]+}} = { {{.*}}"instrument-function-entry-inlined"="\01__gnu_mcount_nc"{{.*}} }
 // CHECK-ARM-EABI-NETBSD: attributes #{{[0-9]+}} = { {{.*}}"instrument-function-entry-inlined"="__mcount"{{.*}} }
 // CHECK-ARM-EABI-NETBSD-NOT: attributes #{{[0-9]+}} = { {{.*}}"instrument-function-entry-inlined"="\01__gnu_mcount_nc"{{.*}} }
 // CHECK-ARM-EABI-OPENBSD: attributes #{{[0-9]+}} = { {{.*}}"instrument-function-entry-inlined"="__mcount"{{.*}} }
 // CHECK-ARM-EABI-OPENBSD-NOT: attributes #{{[0-9]+}} = { {{.*}}"instrument-function-entry-inlined"="\01__gnu_mcount_nc"{{.*}} }
 // CHECK-ARM64-EABI-OPENBSD: attributes #{{[0-9]+}} = { {{.*}}"instrument-function-entry-inlined"="__mcount"{{.*}} }
 // CHECK-ARM64-EABI-OPENBSD-NOT: attributes #{{[0-9]+}} = { {{.*}}"instrument-function-entry-inlined"="\01__gnu_mcount_nc"{{.*}} }
 // CHECK-ARM-EABI-MEABI-GNU-NOT: attributes #{{[0-9]+}} = { {{.*}}"instrument-function-entry-inlined"="mcount"{{.*}} }
-// CHECK-ARM-EABI-MEABI-GNU: attributes #{{[0-9]+}} = { {{.*}}"instrument-function-entry-inlined"="\01__gnu_mcount_nc"{{.*}} }
+// CHECK-ARM-EABI-MEABI-GNU: attributes #{{[0-9]+}} = { {{.*}}"instrument-function-entry-inlined"="llvm.arm.gnu.eabi.mcount"{{.*}} }
 // CHECK-ARM-EABI-RTEMS: attributes #{{[0-9]+}} = { {{.*}}"instrument-function-entry-inlined"="mcount"{{.*}} }
 // CHECK-ARM-EABI-RTEMS-NOT: attributes #{{[0-9]+}} = { {{.*}}"instrument-function-entry-inlined"="\01__gnu_mcount_nc"{{.*}} }
 // CHECK-ARM64-EABI-RTEMS: attributes #{{[0-9]+}} = { {{.*}}"instrument-function-entry-inlined"="mcount"{{.*}} }
 // CHECK-ARM64-EABI-RTEMS-NOT: attributes #{{[0-9]+}} = { {{.*}}"instrument-function-entry-inlined"="\01__gnu_mcount_nc"{{.*}} }
 // CHECK-ARM-EABI-CLOUDABI: attributes #{{[0-9]+}} = { {{.*}}"instrument-function-entry-inlined"="mcount"{{.*}} }
 // CHECK-ARM-EABI-CLOUDABI-NOT: attributes #{{[0-9]+}} = { {{.*}}"instrument-function-entry-inlined"="\01__gnu_mcount_nc"{{.*}} }
 // CHECK-ARM64-EABI-CLOUDABI: attributes #{{[0-9]+}} = { {{.*}}"instrument-function-entry-inlined"="mcount"{{.*}} }
 // CHECK-ARM64-EABI-CLOUDABI-NOT: attributes #{{[0-9]+}} = { {{.*}}"instrument-function-entry-inlined"="\01__gnu_mcount_nc"{{.*}} }
 

llvm/include/llvm/IR/IntrinsicsARM.td

class Neon_Dot_Intrinsic
   : Intrinsic<[llvm_anyvector_ty],
               [LLVMMatchType<0>, llvm_anyvector_ty,
                LLVMMatchType<1>],
               [IntrNoMem]>;
 def int_arm_neon_udot : Neon_Dot_Intrinsic;
 def int_arm_neon_sdot : Neon_Dot_Intrinsic;
 
 
+// GNU eabi mcount
+def int_arm_gnu_eabi_mcount : Intrinsic<[],
+                                    [],
+                                    [IntrReadMem, IntrWriteMem]>;
+
 } // end TargetPrefix

llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp

bool ARMExpandPseudo::ExpandCMP_SWAP_64(MachineBasicBlock &MBB,
   return true;
 }
 
 
 bool ARMExpandPseudo::ExpandMI(MachineBasicBlock &MBB,
                                MachineBasicBlock::iterator MBBI,
                                MachineBasicBlock::iterator &NextMBBI) {
   MachineInstr &MI = *MBBI;
   unsigned Opcode = MI.getOpcode();
   switch (Opcode) {
     default:
       return false;
 
     case ARM::TCRETURNdi:
     case ARM::TCRETURNri: {
       MachineBasicBlock::iterator MBBI = MBB.getLastNonDebugInstr();
       assert(MBBI->isReturn() &&
case ARM::CMP_SWAP_32:
       if (STI->isThumb())
         return ExpandCMP_SWAP(MBB, MBBI, ARM::t2LDREX, ARM::t2STREX, 0,
                               NextMBBI);
       else
         return ExpandCMP_SWAP(MBB, MBBI, ARM::LDREX, ARM::STREX, 0, NextMBBI);
 
     case ARM::CMP_SWAP_64:
       return ExpandCMP_SWAP_64(MBB, MBBI, NextMBBI);
+
+    case ARM::tBL_PUSHLR:
+    case ARM::BL_PUSHLR: {
+      const bool Thumb = Opcode == ARM::tBL_PUSHLR;
+      Register Reg = MI.getOperand(0).getReg();
+      assert(Reg == ARM::LR && "expect LR register!");
+      MachineInstrBuilder MIB;
+      if (Thumb) {
+        // push {lr}
+        BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(ARM::tPUSH))
+            .add(predOps(ARMCC::AL))
+            .addReg(Reg);
+
+        // bl __gnu_mcount_nc
+        MIB = BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(ARM::tBL));
+      } else {
+        // stmdb   sp!, {lr}
+        BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(ARM::STMDB_UPD))
+            .addReg(ARM::SP, RegState::Define)
+            .addReg(ARM::SP)
+            .add(predOps(ARMCC::AL))
+            .addReg(Reg);
+
+        // bl __gnu_mcount_nc
+        MIB = BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(ARM::BL));
+      }
+      MIB.cloneMemRefs(MI);
+      for (unsigned i = 1; i < MI.getNumOperands(); ++i) MIB.add(MI.getOperand(i));
+      MI.eraseFromParent();
+      return true;
+    }
   }
 }
 
 bool ARMExpandPseudo::ExpandMBB(MachineBasicBlock &MBB) {
   bool Modified = false;
 
   MachineBasicBlock::iterator MBBI = MBB.begin(), E = MBB.end();
   while (MBBI != E) {
     MachineBasicBlock::iterator NMBBI = std::next(MBBI);

llvm/lib/Target/ARM/ARMISelLowering.h

CCAssignFn *CCAssignFnForNode(CallingConv::ID CC, bool Return,
                                   bool isVarArg) const;
     SDValue LowerMemOpCallTo(SDValue Chain, SDValue StackPtr, SDValue Arg,
                              const SDLoc &dl, SelectionDAG &DAG,
                              const CCValAssign &VA,
                              ISD::ArgFlagsTy Flags) const;
     SDValue LowerEH_SJLJ_SETJMP(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerEH_SJLJ_LONGJMP(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerEH_SJLJ_SETUP_DISPATCH(SDValue Op, SelectionDAG &DAG) const;
+    SDValue LowerINTRINSIC_VOID(SDValue Op, SelectionDAG &DAG,
+                                    const ARMSubtarget *Subtarget) const;
     SDValue LowerINTRINSIC_WO_CHAIN(SDValue Op, SelectionDAG &DAG,
                                     const ARMSubtarget *Subtarget) const;
     SDValue LowerBlockAddress(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerConstantPool(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerGlobalAddress(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerGlobalAddressDarwin(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerGlobalAddressELF(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerGlobalAddressWindows(SDValue Op, SelectionDAG &DAG) const;

llvm/lib/Target/ARM/ARMISelLowering.cpp

if (Subtarget->isThumb1Only() || !Subtarget->hasV6Ops()
       || (Subtarget->isThumb2() && !Subtarget->hasDSP()))
     setOperationAction(ISD::MULHS, MVT::i32, Expand);
 
   setOperationAction(ISD::SHL_PARTS, MVT::i32, Custom);
   setOperationAction(ISD::SRA_PARTS, MVT::i32, Custom);
   setOperationAction(ISD::SRL_PARTS, MVT::i32, Custom);
   setOperationAction(ISD::SRL,       MVT::i64, Custom);
   setOperationAction(ISD::SRA,       MVT::i64, Custom);
+  setOperationAction(ISD::INTRINSIC_VOID, MVT::Other, Custom);
   setOperationAction(ISD::INTRINSIC_WO_CHAIN, MVT::i64, Custom);
 
   // MVE lowers 64 bit shifts to lsll and lsrl
   // assuming that ISD::SRL and SRA of i64 are already marked custom
   if (Subtarget->hasMVEIntegerOps())
     setOperationAction(ISD::SHL, MVT::i64, Custom);
 
   // Expand to __aeabi_l{lsl,lsr,asr} calls for Thumb1.
else
       CallOpc = isLocalARMFunc ? ARMISD::CALL_PRED : ARMISD::CALL;
   }
 
   std::vector<SDValue> Ops;
   Ops.push_back(Chain);
   Ops.push_back(Callee);
 
   // Add argument registers to the end of the list so that they are known live
   // into the call.
   for (unsigned i = 0, e = RegsToPass.size(); i != e; ++i)
     Ops.push_back(DAG.getRegister(RegsToPass[i].first,
                                   RegsToPass[i].second.getValueType()));
 
   // Add a register mask operand representing the call-preserved registers.
   if (!isTailCall) {
     const uint32_t *Mask;
     const ARMBaseRegisterInfo *ARI = Subtarget->getRegisterInfo();

 
 SDValue ARMTargetLowering::LowerEH_SJLJ_SETUP_DISPATCH(SDValue Op,
                                                       SelectionDAG &DAG) const {
   SDLoc dl(Op);
   return DAG.getNode(ARMISD::EH_SJLJ_SETUP_DISPATCH, dl, MVT::Other,
                      Op.getOperand(0));
 }
 
+SDValue ARMTargetLowering::LowerINTRINSIC_VOID(
+    SDValue Op, SelectionDAG &DAG, const ARMSubtarget *Subtarget) const {
+  unsigned IntNo =
+      cast<ConstantSDNode>(
+          Op.getOperand(Op.getOperand(0).getValueType() == MVT::Other))
+          ->getZExtValue();
+  switch (IntNo) {
+    default:
+      return SDValue();  // Don't custom lower most intrinsics.
+    case Intrinsic::arm_gnu_eabi_mcount: {
+      MachineFunction &MF = DAG.getMachineFunction();
+      EVT PtrVT = getPointerTy(DAG.getDataLayout());
+      SDLoc dl(Op);
+      SDValue Chain = Op.getOperand(0);
+      // call "\01__gnu_mcount_nc"
+      const ARMBaseRegisterInfo *ARI = Subtarget->getRegisterInfo();
+      const uint32_t *Mask =
+          ARI->getCallPreservedMask(DAG.getMachineFunction(), CallingConv::C);
+      assert(Mask && "Missing call preserved mask for calling convention");
+      // Mark LR an implicit live-in.
+      unsigned Reg = MF.addLiveIn(ARM::LR, getRegClassFor(MVT::i32));
+      SDValue ReturnAddress =
+          DAG.getCopyFromReg(DAG.getEntryNode(), dl, Reg, PtrVT);
+      std::vector<EVT> ResultTys = {MVT::Other, MVT::Glue};
+      SDValue Callee =
+          DAG.getTargetExternalSymbol("\01__gnu_mcount_nc", PtrVT, 0);
+      SDValue RegisterMask = DAG.getRegisterMask(Mask);
+      if (Subtarget->isThumb())
+        return SDValue(
+            DAG.getMachineNode(
+                ARM::tBL_PUSHLR, dl, ResultTys,
+                {ReturnAddress, DAG.getTargetConstant(ARMCC::AL, dl, PtrVT),
+                 DAG.getRegister(0, PtrVT), Callee, RegisterMask, Chain}),
+            0);
+      return SDValue(
+          DAG.getMachineNode(ARM::BL_PUSHLR, dl, ResultTys,
+                             {ReturnAddress, Callee, RegisterMask, Chain}),
+          0);
+    }
+  }
+}
+
 SDValue
 ARMTargetLowering::LowerINTRINSIC_WO_CHAIN(SDValue Op, SelectionDAG &DAG,
                                           const ARMSubtarget *Subtarget) const {
   unsigned IntNo = cast<ConstantSDNode>(Op.getOperand(0))->getZExtValue();
   SDLoc dl(Op);
   switch (IntNo) {
   default: return SDValue();    // Don't custom lower most intrinsics.
   case Intrinsic::thread_pointer: {
SDValue ARMTargetLowering::LowerOperation(SDValue Op, SelectionDAG &DAG) const {
   case ISD::FP_TO_SINT:
   case ISD::FP_TO_UINT:    return LowerFP_TO_INT(Op, DAG);
   case ISD::FCOPYSIGN:     return LowerFCOPYSIGN(Op, DAG);
   case ISD::RETURNADDR:    return LowerRETURNADDR(Op, DAG);
   case ISD::FRAMEADDR:     return LowerFRAMEADDR(Op, DAG);
   case ISD::EH_SJLJ_SETJMP: return LowerEH_SJLJ_SETJMP(Op, DAG);
   case ISD::EH_SJLJ_LONGJMP: return LowerEH_SJLJ_LONGJMP(Op, DAG);
   case ISD::EH_SJLJ_SETUP_DISPATCH: return LowerEH_SJLJ_SETUP_DISPATCH(Op, DAG);
+  case ISD::INTRINSIC_VOID: return LowerINTRINSIC_VOID(Op, DAG, Subtarget);
   case ISD::INTRINSIC_WO_CHAIN: return LowerINTRINSIC_WO_CHAIN(Op, DAG,
                                                                Subtarget);
   case ISD::BITCAST:       return ExpandBITCAST(Op.getNode(), DAG, Subtarget);
   case ISD::SHL:
   case ISD::SRL:
   case ISD::SRA:           return LowerShift(Op.getNode(), DAG, Subtarget);
   case ISD::SREM:          return LowerREM(Op.getNode(), DAG);
   case ISD::UREM:          return LowerREM(Op.getNode(), DAG);

llvm/lib/Target/ARM/ARMInstrInfo.td

def BMOVPCRX_CALL : ARMPseudoInst<(outs), (ins tGPR:$func),
                    8, IIC_Br, [(ARMcall_nolink tGPR:$func)]>,
                    Requires<[IsARM, NoV4T]>, Sched<[WriteBr]>;
 
   // mov lr, pc; b if callee is marked noreturn to avoid confusing the
   // return stack predictor.
   def BMOVPCB_CALL : ARMPseudoInst<(outs), (ins arm_bl_target:$func),
                                8, IIC_Br, [(ARMcall_nolink tglobaladdr:$func)]>,
                       Requires<[IsARM]>, Sched<[WriteBr]>;
+
+  // push lr before the call
+  def BL_PUSHLR : ARMPseudoInst<(outs), (ins GPRlr:$ra, arm_bl_target:$func),
+                  4, IIC_Br,
+                  []>,
+             Requires<[IsARM]>, Sched<[WriteBr]>;
 }
 
 let isBranch = 1, isTerminator = 1 in {
   // FIXME: should be able to write a pattern for ARMBrcond, but can't use
   // a two-value operand where a dag node expects two operands. :(
   def Bcc : ABI<0b1010, (outs), (ins arm_br_target:$target),
                IIC_Br, "b", "\t$target",
                [/*(ARMbrcond bb:$target, imm:$cc, CCR:$ccr)*/]>,

llvm/lib/Target/ARM/ARMInstrThumb.td

def tBLXNSr : TI<(outs), (ins pred:$p, GPRnopc:$func), IIC_Br,
     let Unpredictable{1-0} = 0b11;
   }
 
   // ARMv4T
   def tBX_CALL : tPseudoInst<(outs), (ins tGPR:$func),
                   4, IIC_Br,
                   [(ARMcall_nolink tGPR:$func)]>,
             Requires<[IsThumb, IsThumb1Only]>, Sched<[WriteBr]>;
+
+  // Also used for Thumb2
+  // push lr before the call
+  def tBL_PUSHLR : tPseudoInst<(outs), (ins GPRlr:$ra, pred:$p, thumb_bl_target:$func),
+                  4, IIC_Br,
+                  []>,
+             Requires<[IsThumb]>, Sched<[WriteBr]>;
 }
 
 let isBranch = 1, isTerminator = 1, isBarrier = 1 in {
   let isPredicable = 1 in
   def tB   : T1pI<(outs), (ins t_brtarget:$target), IIC_Br,
                  "b", "\t$target", [(br bb:$target)]>,
              T1Encoding<{1,1,1,0,0,?}>, Sched<[WriteBr]> {
     bits<11> target;

llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp

 
 static void insertCall(Function &CurFn, StringRef Func,
                        Instruction *InsertionPt, DebugLoc DL) {
   Module &M = *InsertionPt->getParent()->getParent()->getParent();
   LLVMContext &C = InsertionPt->getParent()->getContext();
 
   if (Func == "mcount" ||
       Func == ".mcount" ||
-      Func == "\01__gnu_mcount_nc" ||
+      Func == "llvm.arm.gnu.eabi.mcount" ||
       Func == "\01_mcount" ||
       Func == "\01mcount" ||
       Func == "__mcount" ||
       Func == "_mcount" ||
       Func == "__cyg_profile_func_enter_bare") {
     FunctionCallee Fn = M.getOrInsertFunction(Func, Type::getVoidTy(C));
     CallInst *Call = CallInst::Create(Fn, "", InsertionPt);
     Call->setDebugLoc(DL);

llvm/test/CodeGen/ARM/gnu_mcount_nc.ll

+; RUN: llc -mtriple=armv7a-linux-gnueabihf -verify-machineinstrs %s -o - | FileCheck %s --check-prefix=CHECK-ARM
+; RUN: llc -mtriple=armv7a-linux-gnueabihf -verify-machineinstrs -fast-isel %s -o - | FileCheck %s --check-prefix=CHECK-ARM-FAST-ISEL
+; RUN: llc -mtriple=armv7a-linux-gnueabihf -verify-machineinstrs -global-isel -global-isel-abort=2 %s -o - | FileCheck %s --check-prefix=CHECK-ARM-GLOBAL-ISEL
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf -verify-machineinstrs %s -o - | FileCheck %s --check-prefix=CHECK-THUMB
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf -verify-machineinstrs -fast-isel %s -o - | FileCheck %s --check-prefix=CHECK-THUMB-FAST-ISEL
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf -verify-machineinstrs -global-isel -global-isel-abort=2 %s -o - | FileCheck %s --check-prefix=CHECK-THUMB-GLOBAL-ISEL
+
+define dso_local void @callee() #0 {
+; CHECK-ARM:                    stmdb   sp!, {lr}
+; CHECK-ARM-NEXT:               bl      __gnu_mcount_nc
+; CHECK-ARM-FAST-ISEL:          stmdb   sp!, {lr}
+; CHECK-ARM-FAST-ISEL-NEXT:     bl      __gnu_mcount_nc
+; CHECK-ARM-GLOBAL-ISEL:        stmdb   sp!, {lr}
+; CHECK-ARM-GLOBAL-ISEL-NEXT:   bl      __gnu_mcount_nc
+; CHECK-THUMB:                  push    {lr}
+; CHECK-THUMB-NEXT:             bl      __gnu_mcount_nc
+; CHECK-THUMB-FAST-ISEL:        push    {lr}
+; CHECK-THUMB-FAST-ISEL-NEXT:   bl      __gnu_mcount_nc
+; CHECK-THUMB-GLOBAL-ISEL:      push    {lr}
+; CHECK-THUMB-GLOBAL-ISEL-NEXT: bl      __gnu_mcount_nc
+  ret void
+}
+
+define dso_local void @caller() #0 {
+; CHECK-ARM:                    stmdb   sp!, {lr}
+; CHECK-ARM-NEXT:               bl      __gnu_mcount_nc
+; CHECK-ARM-FAST-ISEL:          stmdb   sp!, {lr}
+; CHECK-ARM-FAST-ISEL-NEXT:     bl      __gnu_mcount_nc
+; CHECK-ARM-GLOBAL-ISEL:        stmdb   sp!, {lr}
+; CHECK-ARM-GLOBAL-ISEL-NEXT:   bl      __gnu_mcount_nc
+; CHECK-THUMB:                  push    {lr}
+; CHECK-THUMB-NEXT:             bl      __gnu_mcount_nc
+; CHECK-THUMB-FAST-ISEL:        push    {lr}
+; CHECK-THUMB-FAST-ISEL-NEXT:   bl      __gnu_mcount_nc
+; CHECK-THUMB-GLOBAL-ISEL:      push    {lr}
+; CHECK-THUMB-GLOBAL-ISEL-NEXT: bl      __gnu_mcount_nc
+  call void @callee()
+  ret void
+}
+
+attributes #0 = { nofree nounwind "instrument-function-entry-inlined"="llvm.arm.gnu.eabi.mcount" }

llvm/test/Transforms/EntryExitInstrumenter/mcount.ll

 ; The mcount function has many different names.
 
 define void @f1() #1 { entry: ret void }
 ; CHECK-LABEL: define void @f1
 ; CHECK: call void @.mcount
 
 define void @f2() #2 { entry: ret void }
 ; CHECK-LABEL: define void @f2
-; CHECK: call void @"\01__gnu_mcount_nc"
+; CHECK: call void @llvm.arm.gnu.eabi.mcount
 
 define void @f3() #3 { entry: ret void }
 ; CHECK-LABEL: define void @f3
 ; CHECK: call void @"\01_mcount"
 
 define void @f4() #4 { entry: ret void }
 ; CHECK-LABEL: define void @f4
 ; CHECK: call void @"\01mcount"

 }
 
 ; The attributes are "consumed" when the instrumentation is inserted.
 ; CHECK: attributes
 ; CHECK-NOT: instrument-function
 
 attributes #0 = { "instrument-function-entry-inlined"="mcount" "instrument-function-entry"="__cyg_profile_func_enter" "instrument-function-exit"="__cyg_profile_func_exit" }
 attributes #1 = { "instrument-function-entry-inlined"=".mcount" }
-attributes #2 = { "instrument-function-entry-inlined"="\01__gnu_mcount_nc" }
+attributes #2 = { "instrument-function-entry-inlined"="llvm.arm.gnu.eabi.mcount" }
 attributes #3 = { "instrument-function-entry-inlined"="\01_mcount" }
 attributes #4 = { "instrument-function-entry-inlined"="\01mcount" }
 attributes #5 = { "instrument-function-entry-inlined"="__mcount" }
 attributes #6 = { "instrument-function-entry-inlined"="_mcount" }
 attributes #7 = { "instrument-function-entry-inlined"="__cyg_profile_func_enter_bare" }
 attributes #8 = { "instrument-function-exit"="__cyg_profile_func_exit" }