[TwoAddressInstructionPass] Improve tryInstructionCommute X86 FMA and vpternlog instructions

[craig.topper]

Summary

These instructions have 3 operands that can be commuted. The first commute we find may not be the best. So we should keep searching even after we perform a commute.

[craig.topper]

Don't continue search if we found a non-aggressive commute. We don't want an aggressive commute to overwrite it.

[v_klochkov]

Hi Craig,
This change-set looks good to me.
-Vyacheslav

lib/CodeGen/TwoAddressInstructionPass.cpp

1386 ↗

(On Diff #137831)

I was looking for some potential problems on the caller side of the routine you changed; have not found anything interesting/wrong there, but noticed this odd line.
If it also seems redundant/nop to you and if you decide removing this line, then I am Ok with that.

llvm/trunk/lib/CodeGen/TwoAddressInstructionPass.cpp

 bool TwoAddressInstructionPass::tryInstructionCommute(MachineInstr *MI,
                                                       unsigned DstOpIdx,
                                                       unsigned BaseOpIdx,
                                                       bool BaseOpKilled,
                                                       unsigned Dist) {
   if (!MI->isCommutable())
     return false;
 
+  bool MadeChange = false;
   unsigned DstOpReg = MI->getOperand(DstOpIdx).getReg();
   unsigned BaseOpReg = MI->getOperand(BaseOpIdx).getReg();
   unsigned OpsNum = MI->getDesc().getNumOperands();
   unsigned OtherOpIdx = MI->getDesc().getNumDefs();
   for (; OtherOpIdx < OpsNum; OtherOpIdx++) {
     // The call of findCommutedOpIndices below only checks if BaseOpIdx
     // and OtherOpIdx are commutable, it does not really search for
     // other commutable operands and does not change the values of passed
     // variables.
     if (OtherOpIdx == BaseOpIdx || !MI->getOperand(OtherOpIdx).isReg() ||
         !TII->findCommutedOpIndices(*MI, BaseOpIdx, OtherOpIdx))
       continue;
 
     unsigned OtherOpReg = MI->getOperand(OtherOpIdx).getReg();
     bool AggressiveCommute = false;
 
     // If OtherOp dies but BaseOp does not, swap the OtherOp and BaseOp
     // operands. This makes the live ranges of DstOp and OtherOp joinable.
-    bool DoCommute =
-        !BaseOpKilled && isKilled(*MI, OtherOpReg, MRI, TII, LIS, false);
+    bool OtherOpKilled = isKilled(*MI, OtherOpReg, MRI, TII, LIS, false);
+    bool DoCommute = !BaseOpKilled && OtherOpKilled;
 
     if (!DoCommute &&
         isProfitableToCommute(DstOpReg, BaseOpReg, OtherOpReg, MI, Dist)) {
       DoCommute = true;
       AggressiveCommute = true;
     }
 
     // If it's profitable to commute, try to do so.
     if (DoCommute && commuteInstruction(MI, DstOpIdx, BaseOpIdx, OtherOpIdx,
                                         Dist)) {
+      MadeChange = true;
       ++NumCommuted;
-      if (AggressiveCommute)
+      if (AggressiveCommute) {
         ++NumAggrCommuted;
-      return true;
+        // There might be more than two commutable operands, update BaseOp and
+        // continue scanning.
+        BaseOpReg = OtherOpReg;
+        BaseOpKilled = OtherOpKilled;
+        continue;
       }
+      // If this was a commute based on kill, we won't do better continuing.
+      return MadeChange;
     }
-  return false;
+  }
+  return MadeChange;
 }
 
 /// For the case where an instruction has a single pair of tied register
 /// operands, attempt some transformations that may either eliminate the tied
 /// operands or improve the opportunities for coalescing away the register copy.
 /// Returns true if no copy needs to be inserted to untie mi's operands
 /// (either because they were untied, or because mi was rescheduled, and will
 /// be visited again later). If the shouldOnlyCommute flag is true, only

llvm/trunk/test/CodeGen/X86/avx512-vpternlog-commute.ll

 ; CHECK-NEXT:    retq
   %res = call <16 x i32> @llvm.x86.avx512.mask.pternlog.d.512(<16 x i32> %x1, <16 x i32> %x0, <16 x i32> %x2, i32 114, i16 -1)
   ret <16 x i32> %res
 }
 
 define <16 x i32> @vpternlog_v16i32_210(<16 x i32> %x0, <16 x i32> %x1, <16 x i32> %x2) {
 ; CHECK-LABEL: vpternlog_v16i32_210:
 ; CHECK:       ## %bb.0:
-; CHECK-NEXT:    vpternlogd $78, %zmm0, %zmm2, %zmm1
-; CHECK-NEXT:    vmovdqa64 %zmm1, %zmm0
+; CHECK-NEXT:    vpternlogd $92, %zmm1, %zmm2, %zmm0
 ; CHECK-NEXT:    retq
   %res = call <16 x i32> @llvm.x86.avx512.mask.pternlog.d.512(<16 x i32> %x2, <16 x i32> %x1, <16 x i32> %x0, i32 114, i16 -1)
   ret <16 x i32> %res
 }
 
 define <16 x i32> @vpternlog_v16i32_012_load0(<16 x i32>* %x0ptr, <16 x i32> %x1, <16 x i32> %x2) {
 ; CHECK-LABEL: vpternlog_v16i32_012_load0:
 ; CHECK:       ## %bb.0:
; CHECK-NEXT:    retq
   %res = call <16 x i32> @llvm.x86.avx512.maskz.pternlog.d.512(<16 x i32> %x1, <16 x i32> %x0, <16 x i32> %x2, i32 114, i16 %mask)
   ret <16 x i32> %res
 }
 
 define <16 x i32> @vpternlog_v16i32_210_maskz(<16 x i32> %x0, <16 x i32> %x1, <16 x i32> %x2, i16 %mask) {
 ; CHECK-LABEL: vpternlog_v16i32_210_maskz:
 ; CHECK:       ## %bb.0:
 ; CHECK-NEXT:    kmovd %edi, %k1
-; CHECK-NEXT:    vpternlogd $78, %zmm0, %zmm2, %zmm1 {%k1} {z}
-; CHECK-NEXT:    vmovdqa64 %zmm1, %zmm0
+; CHECK-NEXT:    vpternlogd $92, %zmm1, %zmm2, %zmm0 {%k1} {z}
 ; CHECK-NEXT:    retq
   %res = call <16 x i32> @llvm.x86.avx512.maskz.pternlog.d.512(<16 x i32> %x2, <16 x i32> %x1, <16 x i32> %x0, i32 114, i16 %mask)
   ret <16 x i32> %res
 }
 
 define <16 x i32> @vpternlog_v16i32_012_load0_maskz(<16 x i32>* %x0ptr, <16 x i32> %x1, <16 x i32> %x2, i16 %mask) {
 ; CHECK-LABEL: vpternlog_v16i32_012_load0_maskz:
 ; CHECK:       ## %bb.0: