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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

I was taking a look at some of the ARM/Thumb tests that were failing if this used TII.isCopyInstr. There are definitely some problems happening in there, with undefined register.



================
Comment at: llvm/include/llvm/CodeGen/Passes.h:29
 class MachineFunctionPass;
+class MachineInstr;
 class MemoryBuffer;
----------------
Is this not needed any more?


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:86
 
+using CopyPredicate =
+    std::function<Optional<DestSourcePair>(const MachineInstr &)>;
----------------
I don't think this is used either.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:654
     // Analyze copies (which don't overlap themselves).
-    if (MI.isCopy() && !TRI->regsOverlap(MI.getOperand(0).getReg(),
-                                         MI.getOperand(1).getReg())) {
-      assert(MI.getOperand(0).getReg().isPhysical() &&
-             MI.getOperand(1).getReg().isPhysical() &&
+    auto CopyOperands = isCopyInstr(MI, *TII, UseCopyInstr);
+    if (CopyOperands) {
----------------
I would use Optional<DestSourcePair> as opposed to auto.


================
Comment at: llvm/test/CodeGen/AArch64/copyprop.ll:1
+; RUN: llc < %s -O3 -mtriple=aarch64-- | FileCheck %s
+
----------------
Can you use update_llc_test_checks on the file, to show the full output.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125335