[PATCH] D125335: Give option to use isCopyInstr to determine which MI is treated as Copy instruction in MCP

[Xin Tong via Phabricator via llvm-commits]

adriantong1024 added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:89
+
+static cl::opt<bool> MCPUseCopyInstr("mcp-use-is-copy-instr", cl::init(false),
+                                     cl::Hidden);
----------------
lkail wrote:
> adriantong1024 wrote:
> > lkail wrote:
> > > Why do we need this option? Can we extend MCP to check `TII.isCopyInstr` by default?
> > I've tried to use isCopyInstr by default, but there are cases which it does not work for some architecture. 
> > 
> > e.g. ARM
> > $lr = tMOVr killed $r1, 14, $noreg
> > tBX_RET 14, $noreg, implicit $r0
> > 
> > isCopyInstr sees tMOVr as a COPY instruction, but the use of $lr is not represented in tBX_RET, so the tMOVr is eliminated.
> > 
> > So far such problems have not been seen in AArch64.
> I mean first we can check `MI.isCopy()`, if `MI.isCopy()` returns `false`, we do further check via `TII->isCopyInstr(MI)`. For the initial draft, we can do simple feasibility check(sufficient to cover your case) on `MI` which `TII->isCopyInstr(MI)` returns `true`.
This is what isCopyInstr does (https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/CodeGen/TargetInstrInfo.h#L1025)

The problem (my concern) here is some architecture may not be ready to use isCopyInstr to identify what a COPY is. We could slowly enable it for more and more architectures once this option in place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125335