Fix for PR23103. Correctly propagate the 'IsUndef' flag to the register operands of a commuted instruction.

[andreadb]

Summary

Revision 220239 exposed a latent bug in method 'TargetInstrInfo::commuteInstruction'.

When commuting the operands of a machine instruction, method 'commuteInstruction' doesn't correctly propagate the 'IsUndef' flag to the register operands of the new (commuted) instruction.

Before this patch, the following instruction:

%vreg4<def> = VADDSDrr  %vreg14, %vreg5<undef>; FR64:%vreg4,%vreg14,%vreg5

was wrongly converted by method 'commuteInstruction' into:

%vreg4<def> = VADDSDrr  %vreg5, %vreg14<undef>; FR64:%vreg4,%vreg5,%vreg14

The correct instruction should have been:

%vreg4<def> = VADDSDrr  %vreg5<undef>, %vreg14; FR64:%vreg4,%vreg5,%vreg14

This patch fixes the problem in method 'TargetInstrInfo::commuteInstruction'. When swapping the operands of a machine instruction, we now make sure that 'IsUndef' flags are correctly set.
This fixes PR23103.

Added test case 'pr23103.ll' (obtained from the reproducible posted in PR23103).

Please let me know if ok to submit.

Thanks,
Andrea

[qcolombet]

Hi Andrea,

Looks good to me with a nit on the test case.

Thanks,
-Quentin

test/CodeGen/X86/pr23103.ll

1 ↗

(On Diff #24750)

Please add a few check lines, so to be sure we generate something sensible.

[andreadb]

Thanks for the review Quentin!

I managed to further simplify the test case.
I also added check lines to verify the output assembly as you suggested.

Committed revision 236258.

llvm/trunk/lib/CodeGen/TargetInstrInfo.cpp

MachineInstr *TargetInstrInfo::commuteInstruction(MachineInstr *MI,
   unsigned Reg0 = HasDef ? MI->getOperand(0).getReg() : 0;
   unsigned Reg1 = MI->getOperand(Idx1).getReg();
   unsigned Reg2 = MI->getOperand(Idx2).getReg();
   unsigned SubReg0 = HasDef ? MI->getOperand(0).getSubReg() : 0;
   unsigned SubReg1 = MI->getOperand(Idx1).getSubReg();
   unsigned SubReg2 = MI->getOperand(Idx2).getSubReg();
   bool Reg1IsKill = MI->getOperand(Idx1).isKill();
   bool Reg2IsKill = MI->getOperand(Idx2).isKill();
+  bool Reg1IsUndef = MI->getOperand(Idx1).isUndef();
+  bool Reg2IsUndef = MI->getOperand(Idx2).isUndef();
   // If destination is tied to either of the commuted source register, then
   // it must be updated.
   if (HasDef && Reg0 == Reg1 &&
       MI->getDesc().getOperandConstraint(Idx1, MCOI::TIED_TO) == 0) {
     Reg2IsKill = false;
     Reg0 = Reg2;
     SubReg0 = SubReg2;
   } else if (HasDef && Reg0 == Reg2 &&
if (HasDef) {
     MI->getOperand(0).setSubReg(SubReg0);
   }
   MI->getOperand(Idx2).setReg(Reg1);
   MI->getOperand(Idx1).setReg(Reg2);
   MI->getOperand(Idx2).setSubReg(SubReg1);
   MI->getOperand(Idx1).setSubReg(SubReg2);
   MI->getOperand(Idx2).setIsKill(Reg1IsKill);
   MI->getOperand(Idx1).setIsKill(Reg2IsKill);
+  MI->getOperand(Idx2).setIsUndef(Reg1IsUndef);
+  MI->getOperand(Idx1).setIsUndef(Reg2IsUndef);
   return MI;
 }
 
 /// findCommutedOpIndices - If specified MI is commutable, return the two
 /// operand indices that would swap value. Return true if the instruction
 /// is not in a form which this routine understands.
 bool TargetInstrInfo::findCommutedOpIndices(MachineInstr *MI,
                                             unsigned &SrcOpIdx1,

llvm/trunk/test/CodeGen/X86/pr23103.ll

+; RUN: llc -verify-machineinstrs -mtriple=x86_64-unknown-unknown -mcpu=generic -mattr=+avx < %s | FileCheck %s
+
+; When commuting a VADDSDrr instruction, verify that the 'IsUndef' flag is
+; correctly propagated to the operands of the resulting instruction.
+; Test for PR23103;
+
+declare zeroext i1 @foo(<1 x double>)
+
+define <1 x double> @pr23103(<1 x double>* align 8 %Vp) {
+; CHECK-LABEL: pr23103:
+; CHECK:         vmovsd (%rdi), %xmm0
+; CHECK-NEXT:    vmovsd %xmm0, {{.*}}(%rsp) {{.*#+}} 8-byte Spill
+; CHECK-NEXT:    callq foo
+; CHECK-NEXT:    vaddsd {{.*}}(%rsp), %xmm0, %xmm0 {{.*#+}} 8-byte Folded Reload
+; CHECK:         retq
+entry:
+  %V = load <1 x double>, <1 x double>* %Vp, align 8
+  %call = call zeroext i1 @foo(<1 x double> %V)
+  %fadd = fadd <1 x double> %V, undef
+  ret <1 x double> %fadd
+}