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

[Kristof Beyls via Phabricator via cfe-commits]

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");
   unsigned Opcode = MI.getOpcode();
----------------
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?


================
Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1931-1932
+        // Replace with the pseudo instruction with a call instruction
+        MIB = BuildMI(MBB, MBBI, MI.getDebugLoc(),
+                                          TII->get(ARM::tBL));
+      } else {
----------------
Did you clang-format the patch?


================
Comment at: llvm/test/CodeGen/ARM/gnu_mcount_nc.ll:1-2
+; RUN: llc -mtriple=armv7a-linux-gnueabihf %s -o - | FileCheck %s --check-prefix=CHECK-ARM
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf %s -o - | FileCheck %s --check-prefix=CHECK-THUMB
+
----------------
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)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65019