[X86] Remove what appear to be unnecessary uses of DCI.CombineTo

[craig.topper]

Summary

CombineTo is most useful when you need to replace multiple results, avoid the worklist management, or you need to something else after the combine, etc. Otherwise you should be able to just return the new node and let DAGCombiner go through its usual worklist code.

All of the places changed in this patch look to be standard cases where we should be able to use the more stand behavior of just returning the new node.

[RKSimon]

I think this might have been to avoid the high cost of calling combineX86ShufflesRecursively again - can you see any compile time diffs? Possibly from one of the vector-shuffle-*.ll test files?

[craig.topper]

I'm not sure how it would have avoid calling it again. The "AddTo" argument defaults to true to its going to do

ReplaceAllUses(N, Res);
AddToWorklist(N);
AddUsersToWorklist(N);

[craig.topper]

I ran vector-shuffle-combining.ll with and without the patch with debug output. I got a less visits of things after this patch. It looks like the normal worklist management aggressively removes N and recursively all of its children using DAGCombiner::recursivelyDeleteUnusedNodes. CombineTo on the other hand just puts all the children of N in the Worklist and assumes they'll be deleted if necessary when they get visited. So depending on what's in the worklist some nodes might get visited before the process has gotten a chance to propagate the deadness up the graph.

[RKSimon]

Thanks for checking! LGTM

lib/Target/X86/X86ISelLowering.cpp

case X86ISD::INSERTPS: {
     return SDValue();
   }
   default:
     return SDValue();
   }
 
   // Nuke no-op shuffles that show up after combining.
   if (isNoopShuffleMask(Mask))
-    return DCI.CombineTo(N.getNode(), N.getOperand(0), /*AddTo*/ true);
+    return N.getOperand(0);
 
   // Look for simplifications involving one or two shuffle instructions.
   SDValue V = N.getOperand(0);
   switch (N.getOpcode()) {
   default:
     break;
   case X86ISD::PSHUFLW:
   case X86ISD::PSHUFHW:
if (isTargetShuffle(N->getOpcode())) {
 
     // Try recursively combining arbitrary sequences of x86 shuffle
     // instructions into higher-order shuffles. We do this after combining
     // specific PSHUF instruction sequences into their minimal form so that we
     // can evaluate how many specialized shuffle instructions are involved in
     // a particular chain.
     if (SDValue Res = combineX86ShufflesRecursively(
             {Op}, 0, Op, {0}, {}, /*Depth*/ 1,
-            /*HasVarMask*/ false, DAG, Subtarget)) {
-      return DCI.CombineTo(N, Res);
-    }
+            /*HasVarMask*/ false, DAG, Subtarget))
+      return Res;
   }
 
   return SDValue();
 }
 
 /// Check if a vector extract from a target-specific shuffle of a load can be
 /// folded into a single element load.
 /// Similar handling for VECTOR_SHUFFLE is performed by DAGCombiner, but
if ((N0->isUndef() || N->isOnlyUserOf(N0.getNode())) &&
 
     return getConstVector(Bits, Undefs, VT.getSimpleVT(), DAG, SDLoc(N));
   }
 
   // Attempt to combine as shuffle.
   SDValue Op(N, 0);
   if (SDValue Res =
           combineX86ShufflesRecursively({Op}, 0, Op, {0}, {}, /*Depth*/ 1,
-                                        /*HasVarMask*/ false, DAG, Subtarget)) {
-    return DCI.CombineTo(N, Res);
-  }
+                                        /*HasVarMask*/ false, DAG, Subtarget))
+    return Res;
 
   return SDValue();
 }
 
 static SDValue combineVectorShiftImm(SDNode *N, SelectionDAG &DAG,
                                      TargetLowering::DAGCombinerInfo &DCI,
                                      const X86Subtarget &Subtarget) {
   unsigned Opcode = N->getOpcode();
if (ShiftVal.ult(NumSignBits))
       return N00;
   }
 
   // We can decode 'whole byte' logical bit shifts as shuffles.
   if (LogicalShift && (ShiftVal.getZExtValue() % 8) == 0) {
     SDValue Op(N, 0);
     if (SDValue Res = combineX86ShufflesRecursively(
             {Op}, 0, Op, {0}, {}, /*Depth*/ 1,
-            /*HasVarMask*/ false, DAG, Subtarget)) {
-      return DCI.CombineTo(N, Res);
-    }
+            /*HasVarMask*/ false, DAG, Subtarget))
+      return Res;
   }
 
   // Constant Folding.
   APInt UndefElts;
   SmallVector<APInt, 32> EltBits;
   if (N->isOnlyUserOf(N0.getNode()) &&
       getTargetConstantBitsFromNode(N0, NumBitsPerElt, UndefElts, EltBits)) {
     assert(EltBits.size() == VT.getVectorNumElements() &&
assert(
        (N->getOpcode() == X86ISD::PINSRW &&
         N->getValueType(0) == MVT::v8i16)) &&
       "Unexpected vector insertion");
 
   // Attempt to combine PINSRB/PINSRW patterns to a shuffle.
   SDValue Op(N, 0);
   if (SDValue Res =
           combineX86ShufflesRecursively({Op}, 0, Op, {0}, {}, /*Depth*/ 1,
-                                        /*HasVarMask*/ false, DAG, Subtarget)) {
-    return DCI.CombineTo(N, Res);
-  }
+                                        /*HasVarMask*/ false, DAG, Subtarget))
+    return Res;
 
   return SDValue();
 }
 
 /// Recognize the distinctive (AND (setcc ...) (setcc ..)) where both setccs
 /// reference the same FP CMP, and rewrite for CMPEQSS and friends. Likewise for
 /// OR -> CMPNEQSS.
 static SDValue combineCompareEqual(SDNode *N, SelectionDAG &DAG,
static SDValue combineAnd(SDNode *N, SelectionDAG &DAG,
   if (SDValue R = combineAndLoadToBZHI(N, DAG, Subtarget))
     return R;
 
   // Attempt to recursively combine a bitmask AND with shuffles.
   if (VT.isVector() && (VT.getScalarSizeInBits() % 8) == 0) {
     SDValue Op(N, 0);
     if (SDValue Res = combineX86ShufflesRecursively(
             {Op}, 0, Op, {0}, {}, /*Depth*/ 1,
-            /*HasVarMask*/ false, DAG, Subtarget)) {
-      return DCI.CombineTo(N, Res);
-    }
+            /*HasVarMask*/ false, DAG, Subtarget))
+      return Res;
   }
 
   // Attempt to combine a scalar bitmask AND with an extracted shuffle.
   if ((VT.getScalarSizeInBits() % 8) == 0 &&
       N->getOperand(0).getOpcode() == ISD::EXTRACT_VECTOR_ELT &&
       isa<ConstantSDNode>(N->getOperand(0).getOperand(1))) {
     SDValue BitMask = N->getOperand(1);
     SDValue SrcVec = N->getOperand(0).getOperand(0);
static SDValue combineAndnp(SDNode *N, SelectionDAG &DAG,
 
   EVT VT = N->getValueType(0);
 
   // Attempt to recursively combine a bitmask ANDNP with shuffles.
   if (VT.isVector() && (VT.getScalarSizeInBits() % 8) == 0) {
     SDValue Op(N, 0);
     if (SDValue Res = combineX86ShufflesRecursively(
             {Op}, 0, Op, {0}, {}, /*Depth*/ 1,
-            /*HasVarMask*/ false, DAG, Subtarget)) {
-      return DCI.CombineTo(N, Res);
-    }
+            /*HasVarMask*/ false, DAG, Subtarget))
+      return Res;
   }
 
   return SDValue();
 }
 
 static SDValue combineBT(SDNode *N, SelectionDAG &DAG,
                          TargetLowering::DAGCombinerInfo &DCI) {
   SDValue N0 = N->getOperand(0);