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