GlobalISel: Fix matchEqualDefs for instructions with multiple defs

[Petar.Avramovic]

Summary

Instructions that produceSameValue produce same values for operands with
same index. matchEqualDefs used to return true for any two values from
different instructions that produce same values. Fix this by checking if
values are defined by operands with the same index.

[foad]

Is this a bug fix or just a missed optimization? Surely matchEqualDefs is not required to return true when two defs have the same value?

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

2736

Maybe your new code ought to go here? And maybe it should apply to any instruction with multiple return registers, not just unmerge?

[Petar.Avramovic]

Merge the same instructions case with different instructions that produce same values.
Check for all instructions with multiple defs not only g_unmerge.

[Petar.Avramovic]

This is bug fix, matchEqualDefs always returns true when different I1 and I2 produce same values (missing operand index check).

[foad]

The bug fix looks good to me. Inline comments are just about optimization.

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

2671–2676

I'm not sure if it's a good idea to remove this code.

• I think it's correct(?)
• It looks like a useful speed optimization if I1 and I2 happen to be the same instruction.
• It allows returning true if I1 and I2 refer to the same load or store, otherwise we'd return false below.

Thoughts?
Adding Stas who wrote this code.

2756

If findRegisterDefOperandIdx is slow then it might be worth adding a fast case like: "if I1 only has 1 def, return true". But I'm not sure if it's worth it.

[Petar.Avramovic]

I didn't think about I1 and I2 being same load.

[Petar.Avramovic]

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

2756

findRegisterDefOperandIdx simply loops through operands, if it is only one def it will match it in first try. Num operands is already calculated so it is probably a little bit faster in theory but I don't think it is worth it.

[foad]

LGTM, thanks!

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

bool CombinerHelper::eraseInst(MachineInstr &MI) {
   MI.eraseFromParent();
   return true;
 }
 
 bool CombinerHelper::matchEqualDefs(const MachineOperand &MOP1,
                                     const MachineOperand &MOP2) {
   if (!MOP1.isReg() || !MOP2.isReg())
     return false;
-  MachineInstr *I1 = getDefIgnoringCopies(MOP1.getReg(), MRI);
-  if (!I1)
+  auto InstAndDef1 = getDefSrcRegIgnoringCopies(MOP1.getReg(), MRI);
+  if (!InstAndDef1)
     return false;
-  MachineInstr *I2 = getDefIgnoringCopies(MOP2.getReg(), MRI);
-  if (!I2)
+  auto InstAndDef2 = getDefSrcRegIgnoringCopies(MOP2.getReg(), MRI);
+  if (!InstAndDef2)
     return false;
+  MachineInstr *I1 = InstAndDef1->MI;
+  MachineInstr *I2 = InstAndDef2->MI;
 
   // Handle a case like this:
   //
   // %0:_(s64), %1:_(s64) = G_UNMERGE_VALUES %2:_(<2 x s64>)
   //
   // Even though %0 and %1 are produced by the same instruction they are not
   // the same values.
   if (I1 == I2)
     return MOP1.getReg() == MOP2.getReg();
 
+  // %0:_(s8), %1:_(s8), %2:_(s8), %3:_(s8) = G_UNMERGE_VALUES %4:_(<4 x s8>)
+  // %5:_(s8), %6:_(s8), %7:_(s8), %8:_(s8) = G_UNMERGE_VALUES %4:_(<4 x s8>)
+  //
+  // I1 and I2 are different instructions but produce same values,
+  // %1 and %6 are same, %1 and %7 are not the same value.
+  if (isa<GUnmerge>(I1) && isa<GUnmerge>(I2)) {
+    auto Unmerge1 = cast<GUnmerge>(I1);
+    auto Unmerge2 = cast<GUnmerge>(I2);
+    if (Unmerge1->getSourceReg() == Unmerge2->getSourceReg()) {
+      return Unmerge1->findRegisterDefOperandIdx(InstAndDef1->Reg) ==
+             Unmerge2->findRegisterDefOperandIdx(InstAndDef2->Reg);
+    }
+    return false;
+  }
+
   // If we have an instruction which loads or stores, we can't guarantee that
   // it is identical.
   //
   // For example, we may have
   //
   // %x1 = G_LOAD %addr (load N from @somewhere)
   // ...
   // call @foo
if (any_of(I1->uses(), [](const MachineOperand &MO) {
     // Check if we have a case like this:
     //
     // %a = COPY $physreg
     // %b = COPY %a
     //
     // In this case, I1 and I2 will both be equal to %a = COPY $physreg.
     // From that, we know that they must have the same value, since they must
     // have come from the same COPY.
     return I1->isIdenticalTo(*I2);
   }
 
   // We don't have any physical registers, so we don't necessarily need the
   // same vreg defs.
   //
   // On the off-chance that there's some target instruction feeding into the
   // instruction, let's use produceSameValue instead of isIdenticalTo.
   return Builder.getTII().produceSameValue(*I1, *I2, &MRI);
 }
 
 bool CombinerHelper::matchConstantOp(const MachineOperand &MOP, int64_t C) {
   if (!MOP.isReg())
     return false;
   // MIPatternMatch doesn't let us look through G_ZEXT etc.
   auto ValAndVReg = getConstantVRegValWithLookThrough(MOP.getReg(), MRI);
   return ValAndVReg && ValAndVReg->Value == C;
 }
 
 bool CombinerHelper::replaceSingleDefInstWithOperand(MachineInstr &MI,
                                                      unsigned OpIdx) {
   assert(MI.getNumExplicitDefs() == 1 && "Expected one explicit def?");
   Register OldReg = MI.getOperand(0).getReg();
   Register Replacement = MI.getOperand(OpIdx).getReg();
   assert(canReplaceReg(OldReg, Replacement, MRI) && "Cannot replace register?");
   MI.eraseFromParent();
   replaceRegWith(MRI, OldReg, Replacement);
   return true;
 }

llvm/test/CodeGen/AMDGPU/GlobalISel/postlegalizercombiner-select.mir

bb.0:
     %1:_(s32) = COPY $vgpr0
     %2:_(s1) = G_TRUNC %1:_(s32)
     %3:_(s32), %4:_(s32) = G_UNMERGE_VALUES %0:_(<2 x s32>)
     %5:_(s32) = G_SELECT %2:_(s1), %3:_, %3:_
     $vgpr0 = COPY %5
     SI_RETURN_TO_EPILOG $vgpr0
 
 ...
+
+---
+name: select_different_result_from_different_unmerge_values_with_the_same_source
+tracksRegLiveness: true
+body: |
+  bb.0:
+    liveins: $vgpr0_vgpr1_vgpr2_vgpr3, $vgpr4
+
+    ; GCN-LABEL: name: select_different_result_from_different_unmerge_values_with_the_same_source
+    ; GCN: liveins: $vgpr0_vgpr1_vgpr2_vgpr3, $vgpr4
+    ; GCN: [[COPY:%[0-9]+]]:_(<4 x s32>) = COPY $vgpr0_vgpr1_vgpr2_vgpr3
+    ; GCN: [[COPY1:%[0-9]+]]:_(s32) = COPY $vgpr4
+    ; GCN: [[TRUNC:%[0-9]+]]:_(s1) = G_TRUNC [[COPY1]](s32)
+    ; GCN: [[UV:%[0-9]+]]:_(s32), [[UV1:%[0-9]+]]:_(s32), [[UV2:%[0-9]+]]:_(s32), [[UV3:%[0-9]+]]:_(s32) = G_UNMERGE_VALUES [[COPY]](<4 x s32>)
+    ; GCN: [[UV4:%[0-9]+]]:_(s32), [[UV5:%[0-9]+]]:_(s32), [[UV6:%[0-9]+]]:_(s32), [[UV7:%[0-9]+]]:_(s32) = G_UNMERGE_VALUES [[COPY]](<4 x s32>)
+    ; GCN: [[SELECT:%[0-9]+]]:_(s32) = G_SELECT [[TRUNC]](s1), [[UV1]], [[UV7]]
+    ; GCN: $vgpr0 = COPY [[SELECT]](s32)
+    %0:_(<4 x s32>) = COPY $vgpr0_vgpr1_vgpr2_vgpr3
+    %1:_(s32) = COPY $vgpr4
+    %2:_(s1) = G_TRUNC %1:_(s32)
+    %3:_(s32), %4:_(s32), %5:_(s32), %6:_(s32) = G_UNMERGE_VALUES %0:_(<4 x s32>)
+    %7:_(s32), %8:_(s32), %9:_(s32), %10:_(s32) = G_UNMERGE_VALUES %0:_(<4 x s32>)
+    %11:_(s32) = G_SELECT %2:_(s1), %4:_, %10:_
+    $vgpr0 = COPY %11
+...
+
+---
+name: select_same_result_from_different_unmerge_values_with_the_same_source
+tracksRegLiveness: true
+body: |
+  bb.0:
+    liveins: $vgpr0_vgpr1_vgpr2_vgpr3, $vgpr4
+
+    ; GCN-LABEL: name: select_same_result_from_different_unmerge_values_with_the_same_source
+    ; GCN: liveins: $vgpr0_vgpr1_vgpr2_vgpr3, $vgpr4
+    ; GCN: [[COPY:%[0-9]+]]:_(<4 x s32>) = COPY $vgpr0_vgpr1_vgpr2_vgpr3
+    ; GCN: [[UV:%[0-9]+]]:_(s32), [[UV1:%[0-9]+]]:_(s32), [[UV2:%[0-9]+]]:_(s32), [[UV3:%[0-9]+]]:_(s32) = G_UNMERGE_VALUES [[COPY]](<4 x s32>)
+    ; GCN: $vgpr0 = COPY [[UV1]](s32)
+    %0:_(<4 x s32>) = COPY $vgpr0_vgpr1_vgpr2_vgpr3
+    %1:_(s32) = COPY $vgpr4
+    %2:_(s1) = G_TRUNC %1:_(s32)
+    %3:_(s32), %4:_(s32), %5:_(s32), %6:_(s32) = G_UNMERGE_VALUES %0:_(<4 x s32>)
+    %7:_(s32), %8:_(s32), %9:_(s32), %10:_(s32) = G_UNMERGE_VALUES %0:_(<4 x s32>)
+    %11:_(s32) = G_SELECT %2:_(s1), %4:_, %8:_
+    $vgpr0 = COPY %11
+...