[X86] Fix updating LiveVariables in convertToThreeAddress

[foad]

Summary

Fix all instances of:

• Bad machine code: Kill missing from LiveVariables ***

in the X86 CodeGen tests with D129213 applied, which adds verification
of LiveIntervals after the TwoAddressInstruction pass runs.

[foad]

llvm/lib/Target/X86/X86InstrInfo.cpp

1390

I'm not proud of this patch because it adds duplicated code like this after almost every call to classifyLEAReg. There may be better ways to do this, by completely rethinking the way that convertToThreeAddress updates LiveVariables.

On the other hand LiveVariables is supposed to go away in favour of LiveIntervals, so does it really matter?

[MatzeB]

Added some suggestions. Either way LGTM

llvm/lib/Target/X86/X86InstrInfo.cpp

1390–1391

Can't we just use the already computed isKill here?

1423–1424

as above

1446–1447

as above

1496–1499

Shouldn't this just correspond to the kill flags used in line 1494?

1540

[foad]

llvm/lib/Target/X86/X86InstrInfo.cpp

1390–1391

Yes this works. I didn't do it that way because it will add duplicates in the Kills lists, because the call to replaceKillInstruction below will also add them (in the case that SrcReg == Src.getReg(), i.e. classifyLEAReg did not create a new register), but that does not seem to cause any problems in practice. What do you prefer?

[MatzeB]

llvm/lib/Target/X86/X86InstrInfo.cpp

1390–1391

I see, makes sense. Maybe hint at it with a comment then?

// Record kill in cases where we don't call `replaceKillInstruction` later.
if (LV && SrcReg != Src.getReg())
  LV->getVarInfo(SrcReg).Kills.push_back(NewMI);

[foad]

Add comments.

llvm/lib/Target/X86/X86InstrInfo.cpp

MachineInstr *X86InstrInfo::convertToThreeAddressWithLEA(unsigned MIOpc,
   MachineInstr *ExtMI =
       BuildMI(MBB, MBBI, MI.getDebugLoc(), get(TargetOpcode::COPY))
           .addReg(Dest, RegState::Define | getDeadRegState(IsDead))
           .addReg(OutRegLEA, RegState::Kill, SubReg);
 
   if (LV) {
     // Update live variables.
     LV->getVarInfo(InRegLEA).Kills.push_back(NewMI);
+    if (InRegLEA2)
+      LV->getVarInfo(InRegLEA2).Kills.push_back(NewMI);
     LV->getVarInfo(OutRegLEA).Kills.push_back(ExtMI);
     if (IsKill)
       LV->replaceKillInstruction(Src, MI, *InsMI);
     if (IsDead)
       LV->replaceKillInstruction(Dest, MI, *ExtMI);
   }
 
   if (LIS) {
if (MI.getNumOperands() > 2)
     if (MI.getOperand(2).isReg() && MI.getOperand(2).isUndef())
       return nullptr;
 
   MachineInstr *NewMI = nullptr;
   Register SrcReg, SrcReg2;
   bool Is64Bit = Subtarget.is64Bit();
 
   bool Is8BitOp = false;
+  unsigned NumRegOperands = 2;
   unsigned MIOpc = MI.getOpcode();
   switch (MIOpc) {
   default: llvm_unreachable("Unreachable!");
   case X86::SHL64ri: {
     assert(MI.getNumOperands() >= 3 && "Unknown shift instruction!");
     unsigned ShAmt = getTruncatedShiftCount(MI, 2);
     if (!isTruncatedShiftCountForLEA(ShAmt)) return nullptr;
 
MachineInstrBuilder MIB =
             .addReg(0)
             .addImm(1LL << ShAmt)
             .addReg(SrcReg, getKillRegState(isKill))
             .addImm(0)
             .addReg(0);
     if (ImplicitOp.getReg() != 0)
       MIB.add(ImplicitOp);
     NewMI = MIB;
-
+    if (LV && SrcReg != Src.getReg())
+      LV->getVarInfo(SrcReg).Kills.push_back(NewMI);
     break;
   }
   case X86::SHL8ri:
     Is8BitOp = true;
     LLVM_FALLTHROUGH;
   case X86::SHL16ri: {
     assert(MI.getNumOperands() >= 3 && "Unknown shift instruction!");
     unsigned ShAmt = getTruncatedShiftCount(MI, 2);
case X86::INC32r: {
     MachineInstrBuilder MIB =
         BuildMI(MF, MI.getDebugLoc(), get(Opc))
             .add(Dest)
             .addReg(SrcReg, getKillRegState(isKill));
     if (ImplicitOp.getReg() != 0)
       MIB.add(ImplicitOp);
 
     NewMI = addOffset(MIB, 1);
+    if (LV && SrcReg != Src.getReg())
+      LV->getVarInfo(SrcReg).Kills.push_back(NewMI);
     break;
   }
   case X86::DEC64r:
   case X86::DEC32r: {
     assert(MI.getNumOperands() >= 2 && "Unknown dec instruction!");
     unsigned Opc = MIOpc == X86::DEC64r ? X86::LEA64r
         : (Is64Bit ? X86::LEA64_32r : X86::LEA32r);
 
     bool isKill;
     MachineOperand ImplicitOp = MachineOperand::CreateReg(0, false);
     if (!classifyLEAReg(MI, Src, Opc, /*AllowSP=*/false, SrcReg, isKill,
                         ImplicitOp, LV, LIS))
       return nullptr;
 
     MachineInstrBuilder MIB = BuildMI(MF, MI.getDebugLoc(), get(Opc))
                                   .add(Dest)
                                   .addReg(SrcReg, getKillRegState(isKill));
     if (ImplicitOp.getReg() != 0)
       MIB.add(ImplicitOp);
 
     NewMI = addOffset(MIB, -1);
-
+    if (LV && SrcReg != Src.getReg())
+      LV->getVarInfo(SrcReg).Kills.push_back(NewMI);
     break;
   }
   case X86::DEC8r:
   case X86::INC8r:
     Is8BitOp = true;
     LLVM_FALLTHROUGH;
   case X86::DEC16r:
   case X86::INC16r:
case X86::ADD32rr_DB: {
 
     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())
-      LV->replaceKillInstruction(SrcReg2, MI, *NewMI);
+    if (LV) {
+      if (SrcReg2 != Src2.getReg())
+        LV->getVarInfo(SrcReg2).Kills.push_back(NewMI);
+      if (SrcReg != SrcReg2 && SrcReg != Src.getReg())
+        LV->getVarInfo(SrcReg).Kills.push_back(NewMI);
+    }
+    NumRegOperands = 3;
     break;
   }
   case X86::ADD8rr:
   case X86::ADD8rr_DB:
     Is8BitOp = true;
     LLVM_FALLTHROUGH;
   case X86::ADD16rr:
   case X86::ADD16rr_DB:
case X86::ADD32ri8_DB: {
 
     MachineInstrBuilder MIB = BuildMI(MF, MI.getDebugLoc(), get(Opc))
                                   .add(Dest)
                                   .addReg(SrcReg, getKillRegState(isKill));
     if (ImplicitOp.getReg() != 0)
       MIB.add(ImplicitOp);
 
     NewMI = addOffset(MIB, MI.getOperand(2));
+    if (LV && SrcReg != Src.getReg())
+      LV->getVarInfo(SrcReg).Kills.push_back(NewMI);
     break;
   }
   case X86::ADD8ri:
   case X86::ADD8ri_DB:
     Is8BitOp = true;
     LLVM_FALLTHROUGH;
   case X86::ADD16ri:
   case X86::ADD16ri8:
case X86::VMOVUPSZrrk:      case X86::VMOVAPSZrrk: {
     case X86::VMOVAPSZrrk:      Opc = X86::VBLENDMPSZrrk;    break;
     }
 
     NewMI = BuildMI(MF, MI.getDebugLoc(), get(Opc))
               .add(Dest)
               .add(MI.getOperand(2))
               .add(Src)
               .add(MI.getOperand(3));
+    NumRegOperands = 4;
     break;
   }
   }
 
   if (!NewMI) return nullptr;
 
   if (LV) {  // Update live variables
-    if (Src.isKill())
-      LV->replaceKillInstruction(Src.getReg(), MI, *NewMI);
-    if (Dest.isDead())
-      LV->replaceKillInstruction(Dest.getReg(), MI, *NewMI);
+    for (unsigned I = 0; I < NumRegOperands; ++I) {
+      MachineOperand &Op = MI.getOperand(I);
+      if (Op.isDead() || Op.isKill())
+        LV->replaceKillInstruction(Op.getReg(), MI, *NewMI);
+    }
   }
 
   MachineBasicBlock &MBB = *MI.getParent();
   MBB.insert(MI.getIterator(), NewMI); // Insert the new inst
 
   if (LIS) {
     LIS->ReplaceMachineInstrInMaps(MI, *NewMI);
     if (SrcReg)