[X86] Special-case ADD of two identical registers in convertToThreeAddress

[foad]

Summary

X86InstrInfo::convertToThreeAddress would convert this:

%1:gr32 = ADD32rr killed %0:gr32(tied-def 0), %0:gr32, implicit-def dead $eflags

to this:

undef %2.sub_32bit:gr64 = COPY killed %0:gr32
undef %3.sub_32bit:gr64_nosp = COPY %0:gr32
%1:gr32 = LEA64_32r killed %2:gr64, 1, killed %3:gr64_nosp, 0, $noreg

Note that in the ADD32rr, %0 was used twice and the first use had a kill
flag, which is MachineInstr::addRegisterKilled does.

In the converted code, each use of %0 is copied to a new reg, and the
first COPY inherits the kill flags from the ADD32rr. This causes
machine verification to fail (if you force it to run after
TwoAddressInstructionPass) because the second COPY uses %0 after it is
killed. Note that machine verification is currently disabled after
TwoAddressInstructionPass but this is a step towards being able to
enable it.

Fix this by not inserting more than one COPY from the same source
register.

[foad]

1. I don't know if it's safe to rely on the kill flag being on the first operand that uses a particular reg. If not I'll rework the patch.
2. I don't know how to test this without removing the false from addPass(&TwoAddressInstructionPassID, false) in TargetPassConfig, which will currently still fail for a few other reasons.

[MatzeB]

On a general note, having the kill flag appear only on one operand seems sketchy to me:

Note that in the ADD32rr, %0 was used twice and the first use had a kill flag, which is MachineInstr::addRegisterKilled does.

Glancing at addRegisterKilled seems to me like it would add the flag to all uses within an instruction (at least for vregs). There is a shortcut that aborts if it finds a pre-existing killflag where it wouldn't continue adding it to the other operands. But if that is the case then some other code beforehand got us into the odd situation of having the kill flag only on some operands.

I think ideally we would enforce that kill flag use is consistent within in instruction (either all uses of the same vreg use or do not use the kill flag). But it seems we are currently not so that would be work cleaning that up and enforcing it with the machine verifier. Anyway that seems out of the scope of this change, so for now we probably have to assume the kill flag is inconsistent between operands.

I don't know if it's safe to rely on the kill flag being on the first operand that uses a particular reg. If not I'll rework the patch.

This seems accidental. I think ideally we would enforce consistency of the flag within an instruction. Short of that we better not assume anything like just the first operand having a kill flag...

I don't know how to test this without removing the false from addPass(&TwoAddressInstructionPassID, false) in TargetPassConfig, which will currently still fail for a few other reasons.

Maybe using llc -run-pass liveintervals,twiaddressinstruction in a .mir test would work?

[foad]

Rebase on a test case that I will pre-commit.

[qcolombet]

I don't know if it's safe to rely on the kill flag being on the first operand that uses a particular reg. If not I'll rework the patch.

This seems accidental. I think ideally we would enforce consistency of the flag within an instruction. Short of that we better not assume anything like just the first operand having a kill flag...

That's definitely accidental and I agree it would be best to enforce that the flag is set on all operands within an instruction but this is, like Matthias said, outside of the scope of this PR.

[foad]

Don't rely on the order of kill flags.

[MatzeB]

Could you try if the suggested edits restore the kill flag in the test?

If yes then LGTM.

llvm/lib/Target/X86/X86InstrInfo.cpp

1213–1214

Can we use MI.killsRegister(SrcReg); here, so things work as expected in case of inconsistent kill-flags as discussed earlier?

1227–1228

llvm/test/CodeGen/X86/twoaddr-mul2.ll

11 ↗

(On Diff #376854)

Why isn't this COPY killed [[COPY]] anymore? Are we loosing a kill-flag now?

[foad]

Switch to MIR test case.

[foad]

llvm/test/CodeGen/X86/twoaddr-mul2.ll

11 ↗

(On Diff #376854)

Because the use is copied from the second use operand in %1:gr32 = ADD32rr killed %0:gr32(tied-def 0), %0:gr32, implicit-def dead $eflags, which does not have the killed flag.

Yes we could implement more code here to try to preserve the kill flag from the first use operand, but I thought it might be better to fix the root cause instead, and work out why the second use operand of the ADD32rr was not killed?

[foad]

Use MI.killsRegister to detect kills even if only some uses of SrcReg
are marked as killed.

[foad]

llvm/lib/Target/X86/X86InstrInfo.cpp

1238

@MatzeB are you happy with this bit? Your suggested changes didn't go quite far enough to get a "kill" on the inserted COPY.

[MatzeB]

Still LGTM

llvm/lib/Target/X86/X86InstrInfo.cpp

1238

Yes makes sense.

.addReg(Src, getKillRegState(isKill)) above.

llvm/lib/Target/X86/X86InstrInfo.cpp

bool X86InstrInfo::classifyLEAReg(MachineInstr &MI, const MachineOperand &Src,
   const TargetRegisterClass *RC;
   if (AllowSP) {
     RC = Opc != X86::LEA32r ? &X86::GR64RegClass : &X86::GR32RegClass;
   } else {
     RC = Opc != X86::LEA32r ?
       &X86::GR64_NOSPRegClass : &X86::GR32_NOSPRegClass;
   }
   Register SrcReg = Src.getReg();
+  isKill = MI.killsRegister(SrcReg);
 
   // For both LEA64 and LEA32 the register already has essentially the right
   // type (32-bit or 64-bit) we may just need to forbid SP.
   if (Opc != X86::LEA64_32r) {
     NewSrc = SrcReg;
-    isKill = Src.isKill();
     assert(!Src.isUndef() && "Undef op doesn't need optimization");
 
     if (NewSrc.isVirtual() && !MF.getRegInfo().constrainRegClass(NewSrc, RC))
       return false;
 
     return true;
   }
 
   // This is for an LEA64_32r and incoming registers are 32-bit. One way or
   // another we need to add 64-bit registers to the final MI.
   if (SrcReg.isPhysical()) {
     ImplicitOp = Src;
     ImplicitOp.setImplicit();
 
-    NewSrc = getX86SubSuperRegister(Src.getReg(), 64);
-    isKill = Src.isKill();
+    NewSrc = getX86SubSuperRegister(SrcReg, 64);
     assert(!Src.isUndef() && "Undef op doesn't need optimization");
   } else {
     // Virtual register of the wrong class, we have to create a temporary 64-bit
     // vreg to feed into the LEA.
     NewSrc = MF.getRegInfo().createVirtualRegister(RC);
     MachineInstr *Copy =
         BuildMI(*MI.getParent(), MI, MI.getDebugLoc(), get(TargetOpcode::COPY))
             .addReg(NewSrc, RegState::Define | RegState::Undef, X86::sub_32bit)
-            .add(Src);
+            .addReg(SrcReg, getKillRegState(isKill));
 
     // Which is obviously going to be dead after we're done with it.
     isKill = true;
 
     if (LV)
       LV->replaceKillInstruction(SrcReg, MI, *Copy);
   }
 
   // We've set all the parameters without issue.
MachineInstr *X86InstrInfo::convertToThreeAddress(MachineInstr &MI,
   case X86::ADD32rr_DB: {
     assert(MI.getNumOperands() >= 3 && "Unknown add instruction!");
     unsigned Opc;
     if (MIOpc == X86::ADD64rr || MIOpc == X86::ADD64rr_DB)
       Opc = X86::LEA64r;
     else
       Opc = Is64Bit ? X86::LEA64_32r : X86::LEA32r;
 
-    bool isKill;
-    Register SrcReg;
-    MachineOperand ImplicitOp = MachineOperand::CreateReg(0, false);
-    if (!classifyLEAReg(MI, Src, Opc, /*AllowSP=*/ true,
-                        SrcReg, isKill, ImplicitOp, LV))
-      return nullptr;
-
     const MachineOperand &Src2 = MI.getOperand(2);
     bool isKill2;
     Register SrcReg2;
     MachineOperand ImplicitOp2 = MachineOperand::CreateReg(0, false);
     if (!classifyLEAReg(MI, Src2, Opc, /*AllowSP=*/ false,
                         SrcReg2, isKill2, ImplicitOp2, LV))
       return nullptr;
 
+    bool isKill;
+    Register SrcReg;
+    MachineOperand ImplicitOp = MachineOperand::CreateReg(0, false);
+    if (Src.getReg() == Src2.getReg()) {
+      // Don't call classify LEAReg a second time on the same register, in case
+      // the first call inserted a COPY from Src2 and marked it as killed.
+      isKill = isKill2;
+      SrcReg = SrcReg2;
+    } else {
+      if (!classifyLEAReg(MI, Src, Opc, /*AllowSP=*/true,
+                          SrcReg, isKill, ImplicitOp, LV))
+        return nullptr;
+    }
+
     MachineInstrBuilder MIB = BuildMI(MF, MI.getDebugLoc(), get(Opc)).add(Dest);
     if (ImplicitOp.getReg() != 0)
       MIB.add(ImplicitOp);
     if (ImplicitOp2.getReg() != 0)
       MIB.add(ImplicitOp2);
 
     NewMI = addRegReg(MIB, SrcReg, isKill, SrcReg2, isKill2);
     if (LV && Src2.isKill())

llvm/test/CodeGen/X86/twoaddr-mul2.mir

 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
-# RUN: llc -mtriple=x86_64-unknown -mcpu=haswell -run-pass=twoaddressinstruction %s -o - | FileCheck %s
+# RUN: llc -mtriple=x86_64-unknown -mcpu=haswell -run-pass=twoaddressinstruction -verify-machineinstrs %s -o - | FileCheck %s
 
-# FIXME: The killed flag should be on the second COPY from [[COPY]], not the first one.
+# Check that we don't have any uses of [[COPY]] after it is killed.
 ---
 name: test_mul_by_2
 tracksRegLiveness: true
 body: |
   bb.0:
     liveins: $edi
 
     ; CHECK-LABEL: name: test_mul_by_2
     ; CHECK: liveins: $edi
     ; CHECK-NEXT: {{  $}}
     ; CHECK-NEXT: [[COPY:%[0-9]+]]:gr32 = COPY killed $edi
-    ; CHECK-NEXT: undef %2.sub_32bit:gr64 = COPY killed [[COPY]]
-    ; CHECK-NEXT: undef %3.sub_32bit:gr64_nosp = COPY [[COPY]]
-    ; CHECK-NEXT: [[LEA64_32r:%[0-9]+]]:gr32 = LEA64_32r killed %2, 1, killed %3, 0, $noreg
+    ; CHECK-NEXT: undef %2.sub_32bit:gr64_nosp = COPY killed [[COPY]]
+    ; CHECK-NEXT: [[LEA64_32r:%[0-9]+]]:gr32 = LEA64_32r killed %2, 1, killed %2, 0, $noreg
     ; CHECK-NEXT: $eax = COPY killed [[LEA64_32r]]
     ; CHECK-NEXT: RET 0, killed $eax
     %0:gr32 = COPY killed $edi
     %1:gr32 = ADD32rr killed %0, %0, implicit-def dead $eflags
     $eax = COPY killed %1
     RET 0, killed $eax
 ...