[X86] Call removeDeadNode when we're done doing custom isel for mul, div and test

[craig.topper]

Summary

Once we've done our custom isel for these nodes, I think we should be calling removeDeadNode to prune them out of the DAG. Table driven isel ultimately either calls morphNodeTo which modifies a node and doesn't leave dead nodes. Or it emits new nodes and then calls removeDeadNode as part of Opc_CompleteMatch.

If you run a simple multiply test case like this through llc with -debug you'll see a umul_lohi node get printed as part of the dump for Instruction Selection ends.

define i64 @foo(i64 %a, i64 %b) local_unnamed_addr #0 {
entry:
  %conv = zext i64 %a to i128
  %conv1 = zext i64 %b to i128
  %mul = mul nuw nsw i128 %conv1, %conv
  %shr = lshr i128 %mul, 64
  %conv2 = trunc i128 %shr to i64
  ret i64 %conv2
}

[niravd]

Yet another case of unused nodes being left behind and causing degradation. We should change the behavior of the DAG ReplaceAllUses-ish functions to check if the old source node becomes unused and if so delete the node. This would obviate the need for this patch and catch the lion share of these sorts of issues but will cause assertions in cases a bunch of cases (which is a better situation than the current silent degradation). I have a first-cut patch (D36643) which fixes just the DAGCombiner CombineTo function and all of the assertion failures from update ordering / double deletions.

In any case, this LGTM.

lib/Target/X86/X86ISelDAGToDAG.cpp

if (!SDValue(Node, 1).use_empty()) {
         ResHi = CurDAG->getCopyFromReg(CurDAG->getEntryNode(), dl, HiReg, NVT,
                                        InFlag);
         InFlag = ResHi.getValue(2);
       }
       ReplaceUses(SDValue(Node, 1), ResHi);
       DEBUG(dbgs() << "=> "; ResHi.getNode()->dump(CurDAG); dbgs() << '\n');
     }
 
+    CurDAG->RemoveDeadNode(Node);
     return;
   }
 
   case ISD::SDIVREM:
   case ISD::UDIVREM:
   case X86ISD::SDIVREM8_SEXT_HREG:
   case X86ISD::UDIVREM8_ZEXT_HREG: {
     SDValue N0 = Node->getOperand(0);
case X86ISD::UDIVREM8_ZEXT_HREG: {
     // Copy the remainder (high) result, if it is needed.
     if (!SDValue(Node, 1).use_empty()) {
       SDValue Result = CurDAG->getCopyFromReg(CurDAG->getEntryNode(), dl,
                                               HiReg, NVT, InFlag);
       InFlag = Result.getValue(2);
       ReplaceUses(SDValue(Node, 1), Result);
       DEBUG(dbgs() << "=> "; Result.getNode()->dump(CurDAG); dbgs() << '\n');
     }
+    CurDAG->RemoveDeadNode(Node);
     return;
   }
 
   case X86ISD::CMP:
   case X86ISD::SUB: {
     // Sometimes a SUB is used to perform comparison.
     if (Opcode == X86ISD::SUB && Node->hasAnyUseOfValue(0))
       // This node is not a CMP.
if ((N0.getOpcode() == ISD::AND ||
 
         // Emit a testb.
         SDNode *NewNode = CurDAG->getMachineNode(X86::TEST8ri, dl, MVT::i32,
                                                  Subreg, Imm);
         // Replace SUB|CMP with TEST, since SUB has two outputs while TEST has
         // one, do not call ReplaceAllUsesWith.
         ReplaceUses(SDValue(Node, (Opcode == X86ISD::SUB ? 1 : 0)),
                     SDValue(NewNode, 0));
+        CurDAG->RemoveDeadNode(Node);
         return;
       }
 
       // For example, "testl %eax, $2048" to "testb %ah, $8".
       if (isShiftedUInt<8, 8>(Mask) &&
           (!(Mask & 0x8000) || hasNoSignedComparisonUses(Node))) {
         // Shift the immediate right by 8 bits.
         SDValue ShiftedImm = CurDAG->getTargetConstant(Mask >> 8, dl, MVT::i8);
if ((N0.getOpcode() == ISD::AND ||
         // target GR8_NOREX registers, so make sure the register class is
         // forced.
         SDNode *NewNode = CurDAG->getMachineNode(X86::TEST8ri_NOREX, dl,
                                                  MVT::i32, Subreg, ShiftedImm);
         // Replace SUB|CMP with TEST, since SUB has two outputs while TEST has
         // one, do not call ReplaceAllUsesWith.
         ReplaceUses(SDValue(Node, (Opcode == X86ISD::SUB ? 1 : 0)),
                     SDValue(NewNode, 0));
+        CurDAG->RemoveDeadNode(Node);
         return;
       }
 
       // For example, "testl %eax, $32776" to "testw %ax, $32776".
       if (isUInt<16>(Mask) && N0.getValueType() != MVT::i16 &&
           (!(Mask & 0x8000) || hasNoSignedComparisonUses(Node))) {
         SDValue Imm = CurDAG->getTargetConstant(Mask, dl, MVT::i16);
         SDValue Reg = N0.getOperand(0);
 
         // Extract the 16-bit subregister.
         SDValue Subreg = CurDAG->getTargetExtractSubreg(X86::sub_16bit, dl,
                                                         MVT::i16, Reg);
 
         // Emit a testw.
         SDNode *NewNode = CurDAG->getMachineNode(X86::TEST16ri, dl, MVT::i32,
                                                  Subreg, Imm);
         // Replace SUB|CMP with TEST, since SUB has two outputs while TEST has
         // one, do not call ReplaceAllUsesWith.
         ReplaceUses(SDValue(Node, (Opcode == X86ISD::SUB ? 1 : 0)),
                     SDValue(NewNode, 0));
+        CurDAG->RemoveDeadNode(Node);
         return;
       }
 
       // For example, "testq %rax, $268468232" to "testl %eax, $268468232".
       if (isUInt<32>(Mask) && N0.getValueType() == MVT::i64 &&
           (!(Mask & 0x80000000) || hasNoSignedComparisonUses(Node))) {
         SDValue Imm = CurDAG->getTargetConstant(Mask, dl, MVT::i32);
         SDValue Reg = N0.getOperand(0);
 
         // Extract the 32-bit subregister.
         SDValue Subreg = CurDAG->getTargetExtractSubreg(X86::sub_32bit, dl,
                                                         MVT::i32, Reg);
 
         // Emit a testl.
         SDNode *NewNode = CurDAG->getMachineNode(X86::TEST32ri, dl, MVT::i32,
                                                  Subreg, Imm);
         // Replace SUB|CMP with TEST, since SUB has two outputs while TEST has
         // one, do not call ReplaceAllUsesWith.
         ReplaceUses(SDValue(Node, (Opcode == X86ISD::SUB ? 1 : 0)),
                     SDValue(NewNode, 0));
+        CurDAG->RemoveDeadNode(Node);
         return;
       }
     }
     break;
   }
   case ISD::STORE:
     if (foldLoadStoreIntoMemOperand(Node))
       return;