[GlobalISel] CombinerHelper: Fix a bug in matchCombineCopy

[volkan]

Summary

When combining COPY instructions, we were replacing the destination registers
with the source register without checking register constraints. This patch adds
a simple logic to check if the constraints match before replacing registers.

[aditya_nandakumar]

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

87

Do we want this method to also handle the case of Regclass to Regclass copies (without any LLTs)? If yes, looks like this early exits.

[volkan]

• Handle the cases where the registers don't have LLT.

[volkan]

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

87

I added support for it as it's currently possible to create a register without an LLT attached, but I'm not sure if it's something we want to support in GlobalISel. What do you all think?

[aditya_nandakumar]

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

87

Thanks. This looks good to me.

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

bool CombinerHelper::tryCombineCopy(MachineInstr &MI) {
   }
   return false;
 }
 bool CombinerHelper::matchCombineCopy(MachineInstr &MI) {
   if (MI.getOpcode() != TargetOpcode::COPY)
     return false;
   Register DstReg = MI.getOperand(0).getReg();
   Register SrcReg = MI.getOperand(1).getReg();
+
+  // Give up if either DstReg or SrcReg  is a physical register.
+  if (Register::isPhysicalRegister(DstReg) ||
+      Register::isPhysicalRegister(SrcReg))
+    return false;
+
+  // Give up the types don't match.
   LLT DstTy = MRI.getType(DstReg);
   LLT SrcTy = MRI.getType(SrcReg);
-  // Simple Copy Propagation.
-  // a(sx) = COPY b(sx) -> Replace all uses of a with b.
-  if (DstTy.isValid() && SrcTy.isValid() && DstTy == SrcTy)
+  // Give up if one has a valid LLT, but the other doesn't.
+  if (DstTy.isValid() != SrcTy.isValid())
+    return false;
+  // Give up if the types don't match.
+  if (DstTy.isValid() && SrcTy.isValid() && DstTy != SrcTy)
+    return false;
+
+  // Get the register banks and classes.
+  const RegisterBank *DstBank = MRI.getRegBankOrNull(DstReg);
+  const RegisterBank *SrcBank = MRI.getRegBankOrNull(SrcReg);
+  const TargetRegisterClass *DstRC = MRI.getRegClassOrNull(DstReg);
+  const TargetRegisterClass *SrcRC = MRI.getRegClassOrNull(SrcReg);
+
+  // Replace if the register constraints match.
+  if ((SrcRC == DstRC) && (SrcBank == DstBank))
     return true;
+  // Replace if DstReg has no constraints.
+  if (!DstBank && !DstRC)
+    return true;
+
   return false;
 }
 void CombinerHelper::applyCombineCopy(MachineInstr &MI) {
   Register DstReg = MI.getOperand(0).getReg();
   Register SrcReg = MI.getOperand(1).getReg();
   MI.eraseFromParent();
   replaceRegWith(MRI, DstReg, SrcReg);
 }

llvm/test/CodeGen/AArch64/GlobalISel/combine-copy.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -o - -march=aarch64 -run-pass=aarch64-prelegalizer-combiner %s | FileCheck %s
+
+# Make sure we don't lose the register bank constraints when
+# combining COPY instructions.
+---
+name:            test_none_none
+body:             |
+  bb.0.entry:
+    ; CHECK-LABEL: name: test_none_none
+    ; CHECK: [[COPY:%[0-9]+]]:_(s64) = COPY $x0
+    ; CHECK: $x0 = COPY [[COPY]](s64)
+    %0:_(s64) = COPY $x0
+    %1:_(s64) = COPY %0(s64)
+    $x0 = COPY %1(s64)
+...
+---
+name:            test_gpr_none
+body:             |
+  bb.0.entry:
+    ; CHECK-LABEL: name: test_gpr_none
+    ; CHECK: [[COPY:%[0-9]+]]:gpr(s64) = COPY $x0
+    ; CHECK: $x0 = COPY [[COPY]](s64)
+    %0:gpr(s64) = COPY $x0
+    %1:_(s64) = COPY %0(s64)
+    $x0 = COPY %1(s64)
+...
+---
+name:            test_none_gpr
+body:             |
+  bb.0.entry:
+    ; CHECK-LABEL: name: test_none_gpr
+    ; CHECK: [[COPY:%[0-9]+]]:_(s64) = COPY $x0
+    ; CHECK: [[COPY1:%[0-9]+]]:gpr(s64) = COPY [[COPY]](s64)
+    ; CHECK: $x0 = COPY [[COPY1]](s64)
+    %0:_(s64) = COPY $x0
+    %1:gpr(s64) = COPY %0(s64)
+    $x0 = COPY %1(s64)
+...
+---
+name:            test_fpr_gpr
+body:             |
+  bb.0.entry:
+    ; CHECK-LABEL: name: test_fpr_gpr
+    ; CHECK: [[COPY:%[0-9]+]]:fpr(s64) = COPY $x0
+    ; CHECK: [[COPY1:%[0-9]+]]:gpr(s64) = COPY [[COPY]](s64)
+    ; CHECK: $x0 = COPY [[COPY1]](s64)
+    %0:fpr(s64) = COPY $x0
+    %1:gpr(s64) = COPY %0(s64)
+    $x0 = COPY %1(s64)
+...
+---
+name:            test_gpr64_gpr64_dst_no_llt
+body:             |
+  bb.0.entry:
+    ; CHECK-LABEL: name: test_gpr64_gpr64_dst_no_llt
+    ; CHECK: [[COPY:%[0-9]+]]:gpr64(s64) = COPY $x0
+    ; CHECK: [[COPY1:%[0-9]+]]:gpr64 = COPY [[COPY]](s64)
+    ; CHECK: $x0 = COPY [[COPY1]]
+    %0:gpr64(s64) = COPY $x0
+    %1:gpr64 = COPY %0(s64)
+    $x0 = COPY %1
+...
+---
+name:            test_gpr64_gpr64_src_no_llt
+body:             |
+  bb.0.entry:
+    ; CHECK-LABEL: name: test_gpr64_gpr64_src_no_llt
+    ; CHECK: [[COPY:%[0-9]+]]:gpr64 = COPY $x0
+    ; CHECK: [[COPY1:%[0-9]+]]:gpr64(s64) = COPY [[COPY]]
+    ; CHECK: $x0 = COPY [[COPY1]](s64)
+    %0:gpr64 = COPY $x0
+    %1:gpr64(s64) = COPY %0
+    $x0 = COPY %1(s64)
+...
+---
+name:            test_gpr64_gpr64_both_no_llt
+body:             |
+  bb.0.entry:
+    ; CHECK-LABEL: name: test_gpr64_gpr64_both_no_llt
+    ; CHECK: [[COPY:%[0-9]+]]:gpr64 = COPY $x0
+    ; CHECK: $x0 = COPY [[COPY]]
+    %0:gpr64 = COPY $x0
+    %1:gpr64 = COPY %0
+    $x0 = COPY %1
+...