[DAGCombiner][X86] Prevent visitSIGN_EXTEND from returning N when (sext (setcc)) already has the target desired type for the setcc

[craig.topper]

Summary

If the setcc already has the target desired type we can reach the getSetCC/getSExtOrTrunc after the MatchingVecType check with the exact same types as the nodes we started with. This causes those causes VsetCC to be CSEd to N0 and the getSExtOrTrunc will CSE to N. When we return N, the caller will think that meant we called CombineTo and did our own worklist management. But that's not what happened. This prevents target hooks from being called for the node.

To fix this, I've now returned SDValue if the setcc is already the desired type. But to avoid some regressions in X86 I've had to disable one of the target combines that wasn't being reached before in the case of a (sext (setcc)). If we get vector widening legalization enabled that entire function will be deleted anyway so hopefully this is only for the short term.

[RKSimon]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

8614 ↗

(On Diff #177305)

Does it matter that this early outs so later combines (matchVSelectOpSizesWithSetCC) can't run?

[craig.topper]

Don't return SDValue, just skip code instead.

[craig.topper]

Ping

[spatel]

lib/Target/X86/X86ISelLowering.cpp

39145 ↗

(On Diff #177459)

Just in case we don't get vector widening legalization done soon, I think this comment should describe the potential regressions and/or what's broken/missing that would prevent that problem.

[craig.topper]

Add explanatory comment for the regressions I'm seeing without the X86ISelLowering change.

[RKSimon]

LGTM

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (N0.getOpcode() == ISD::SETCC) {
     if (VT.isVector() && !LegalOperations &&
         TLI.getBooleanContents(N00VT) ==
             TargetLowering::ZeroOrNegativeOneBooleanContent) {
       // On some architectures (such as SSE/NEON/etc) the SETCC result type is
       // of the same size as the compared operands. Only optimize sext(setcc())
       // if this is the case.
       EVT SVT = getSetCCResultType(N00VT);
 
+      // If we already have the desired type, don't change it.
+      if (SVT != N0.getValueType()) {
         // We know that the # elements of the results is the same as the
         // # elements of the compare (and the # elements of the compare result
         // for that matter).  Check to see that they are the same size.  If so,
         // we know that the element size of the sext'd result matches the
         // element size of the compare operands.
         if (VT.getSizeInBits() == SVT.getSizeInBits())
           return DAG.getSetCC(DL, VT, N00, N01, CC);
 
         // If the desired elements are smaller or larger than the source
         // elements, we can use a matching integer vector type and then
         // truncate/sign extend.
         EVT MatchingVecType = N00VT.changeVectorElementTypeToInteger();
         if (SVT == MatchingVecType) {
           SDValue VsetCC = DAG.getSetCC(DL, MatchingVecType, N00, N01, CC);
           return DAG.getSExtOrTrunc(VsetCC, DL, VT);
         }
       }
+    }
 
     // sext(setcc x, y, cc) -> (select (setcc x, y, cc), T, 0)
     // Here, T can be 1 or -1, depending on the type of the setcc and
     // getBooleanContents().
     unsigned SetCCWidth = N0.getScalarValueSizeInBits();
 
     // To determine the "true" side of the select, we need to know the high bit
     // of the value returned by the setcc if it evaluates to true.

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

if (!Subtarget.hasSSE2())
     return SDValue();
 
   SDValue N0 = N->getOperand(0);
   EVT VT = N->getValueType(0);
   EVT SVT = VT.getScalarType();
   EVT InVT = N0.getValueType();
   EVT InSVT = InVT.getScalarType();
 
+  // FIXME: Generic DAGCombiner previously had a bug that would cause a
+  // sign_extend of setcc to sometimes return the original node and tricked it
+  // into thinking CombineTo was used which prevented the target combines from
+  // running.
+  // Earlying out here to avoid regressions like this
+  //  (v4i32 (sext (v4i1 (setcc (v4i16)))))
+  // Becomes
+  //  (v4i32 (sext_invec (v8i16 (concat (v4i16 (setcc (v4i16))), undef))))
+  // Type legalized to
+  //  (v4i32 (sext_invec (v8i16 (trunc_invec (v4i32 (setcc (v4i32)))))))
+  // Leading to a packssdw+pmovsxwd
+  // We could write a DAG combine to fix this, but really we shouldn't be
+  // creating sext_invec that's forcing v8i16 into the DAG.
+  if (N0.getOpcode() == ISD::SETCC)
+    return SDValue();
+
   // Input type must be a vector and we must be extending legal integer types.
   if (!VT.isVector() || VT.getVectorNumElements() < 2)
     return SDValue();
   if (SVT != MVT::i64 && SVT != MVT::i32 && SVT != MVT::i16)
     return SDValue();
   if (InSVT != MVT::i32 && InSVT != MVT::i16 && InSVT != MVT::i8)
     return SDValue();