[AMDGPU] Don't combine DPP if DPP register is used more than once per instruction

[rampitec]

llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp

364

I assume this exploits the fact we have no instructions which may have more than two uses and are dpp combinable at the same time? If so it deserves a comment.

[vpykhtin]

llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp

364

I'm not sure what do you mean - DPP extension can only be applied to src0 operand.

addUse is called in the order of defuse chain traversal: defusechain_iterator::operator++ assumes that all per instruction uses comes continuosly - so I decided to assume the same.

[rampitec]

llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp

364

Hm... This looks like a hack to me.

[vpykhtin]

llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp

364

It's a cornerstone for MachineRegisterInfo's use_instr_.. iterators, otherwise it wouldn't be cheap to walk.

[foad]

llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp

539–541

It would be much simpler to add a check here that src0 and src1 are not the same register.

[vpykhtin]

Indeed, thanks Jay, updated.

[foad]

llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp

521

This doesn't seem right. Use could be Src2 or some other operand, couldn't it?

[vpykhtin]

llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp

521

I cound't find example for this, but ok, lets check this too.

[vpykhtin]

Rebased, added check if use is Src0 or Src1

[foad]

llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp

522–525

This looks technically OK now. I still think it would be cleaner to structure the code as:

if (Use == Src0) {
  // do it
} else if (Use == Src1 && commutable && Src0 not identical to Src1) {
  // commute and do it
} else {
  // fail
}

But I'll leave that up to your judgement.

523–525

Could use MachineOperand::isIdenticalTo ?

[vpykhtin]

llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp

522–525

I think currently we have better debug messages.

[vpykhtin]

Update before commit: used isIdenticalTo, rebased.

llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp

bool GCNDPPCombine::hasNoImmOrEqual(MachineInstr &MI, unsigned OpndName,
   assert(Imm->isImm());
   return (Imm->getImm() & Mask) == Value;
 }
 
 bool GCNDPPCombine::combineDPPMov(MachineInstr &MovMI) const {
   assert(MovMI.getOpcode() == AMDGPU::V_MOV_B32_dpp);
   LLVM_DEBUG(dbgs() << "\nDPP combine: " << MovMI);
 
   auto *DstOpnd = TII->getNamedOperand(MovMI, AMDGPU::OpName::vdst);
   assert(DstOpnd && DstOpnd->isReg());
   auto DPPMovReg = DstOpnd->getReg();
   if (DPPMovReg.isPhysical()) {
     LLVM_DEBUG(dbgs() << "  failed: dpp move writes physreg\n");
     return false;
   }
   if (execMayBeModifiedBeforeAnyUse(*MRI, DPPMovReg, MovMI)) {
     LLVM_DEBUG(dbgs() << "  failed: EXEC mask should remain the same"
if (TII->isVOP3(OrigOp)) {
         LLVM_DEBUG(dbgs() << "  failed: VOP3 has non-default modifiers\n");
         break;
       }
     } else if (!TII->isVOP1(OrigOp) && !TII->isVOP2(OrigOp)) {
       LLVM_DEBUG(dbgs() << "  failed: not VOP1/2/3\n");
       break;
     }
 
+    auto *Src0 = TII->getNamedOperand(OrigMI, AMDGPU::OpName::src0);
+    auto *Src1 = TII->getNamedOperand(OrigMI, AMDGPU::OpName::src1);
+    if (Use != Src0 && !(Use == Src1 && OrigMI.isCommutable())) { // [1]
+      LLVM_DEBUG(dbgs() << "  failed: no suitable operands\n");
+      break;
+    }
+
+    assert(Src0 && "Src1 without Src0?");
+    if (Src1 && Src1->isIdenticalTo(*Src0)) {
+      assert(Src1->isReg());
+      LLVM_DEBUG(
+          dbgs()
+          << "  " << OrigMI
+          << "  failed: DPP register is used more than once per instruction\n");
+      break;
+    }
+
     LLVM_DEBUG(dbgs() << "  combining: " << OrigMI);
-    if (Use == TII->getNamedOperand(OrigMI, AMDGPU::OpName::src0)) {
+    if (Use == Src0) {
       if (auto *DPPInst = createDPPInst(OrigMI, MovMI, CombOldVGPR,
                                         OldOpndValue, CombBCZ)) {
         DPPMIs.push_back(DPPInst);
         Rollback = false;
       }
-    } else if (OrigMI.isCommutable() &&
-               Use == TII->getNamedOperand(OrigMI, AMDGPU::OpName::src1)) {
+    } else {
+      assert(Use == Src1 && OrigMI.isCommutable()); // by check [1]
       auto *BB = OrigMI.getParent();
       auto *NewMI = BB->getParent()->CloneMachineInstr(&OrigMI);
       BB->insert(OrigMI, NewMI);
       if (TII->commuteInstruction(*NewMI)) {
         LLVM_DEBUG(dbgs() << "  commuted:  " << *NewMI);
         if (auto *DPPInst = createDPPInst(*NewMI, MovMI, CombOldVGPR,
                                           OldOpndValue, CombBCZ)) {
           DPPMIs.push_back(DPPInst);
           Rollback = false;
         }
       } else
         LLVM_DEBUG(dbgs() << "  failed: cannot be commuted\n");
       NewMI->eraseFromParent();
-    } else
-      LLVM_DEBUG(dbgs() << "  failed: no suitable operands\n");
+    }
     if (Rollback)
       break;
     OrigMIs.push_back(&OrigMI);
   }
 
   Rollback |= !Uses.empty();
 
   for (auto *MI : *(Rollback? &DPPMIs : &OrigMIs))

llvm/test/CodeGen/AMDGPU/dpp_combine.mir

bb.0:
     %1:vgpr_32 = COPY $vgpr1
     %2:vgpr_32 = IMPLICIT_DEF
 
     %3:vgpr_32 = V_MOV_B32_dpp undef %2, %1, 1, 15, 15, 1, implicit $exec
     %4:vgpr_32 = nofpexcept nnan V_ADD_F32_e64 0, %3, 0, %0, 0, 0, implicit $mode, implicit $exec
     S_ENDPGM 0, implicit %4
 
 ...
+
+# GCN-LABEL: name: dont_combine_more_than_one_operand
+# GCN: %3:vgpr_32 = V_MAX_F32_e64 0, %2, 0, %2, 0, 0, implicit $mode, implicit $exec
+name: dont_combine_more_than_one_operand
+tracksRegLiveness: true
+body: |
+  bb.0:
+    liveins: $vgpr0, $vgpr1
+    %0:vgpr_32 = COPY $vgpr0
+    %1:vgpr_32 = COPY $vgpr1
+    %2:vgpr_32 = V_MOV_B32_dpp %0, %1, 1, 15, 15, 1, implicit $exec
+    %3:vgpr_32 = V_MAX_F32_e64 0, %2, 0, %2, 0, 0, implicit $mode, implicit $exec
+...
+
+# GCN-LABEL: name: dont_combine_more_than_one_operand_dpp_reg_sequence
+# GCN: %5:vgpr_32 = V_ADD_I32_e32 %4.sub0, %4.sub0, implicit-def $vcc, implicit $exec
+# GCN: %6:vgpr_32 = V_ADDC_U32_e32 %4.sub1, %4.sub1, implicit-def $vcc, implicit $vcc, implicit $exec
+name: dont_combine_more_than_one_operand_dpp_reg_sequence
+tracksRegLiveness: true
+body: |
+  bb.0:
+    liveins: $vgpr0_vgpr1, $vgpr2_vgpr3
+    %0:vreg_64 = COPY $vgpr0_vgpr1
+    %1:vreg_64 = COPY $vgpr2_vgpr3
+    %2:vgpr_32 = V_MOV_B32_dpp %0.sub0, %1.sub0, 1, 15, 15, 1, implicit $exec
+    %3:vgpr_32 = V_MOV_B32_dpp %0.sub1, %1.sub1, 1, 15, 15, 1, implicit $exec
+    %4:vreg_64 = REG_SEQUENCE %2, %subreg.sub0, %3, %subreg.sub1
+    %5:vgpr_32 = V_ADD_I32_e32 %4.sub0, %4.sub0, implicit-def $vcc, implicit $exec
+    %6:vgpr_32 = V_ADDC_U32_e32 %4.sub1, %4.sub1, implicit-def $vcc, implicit $vcc, implicit $exec
+...