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

Craig Topper via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Dec 7 14:04:41 PST 2018


craig.topper created this revision.
craig.topper added reviewers: RKSimon, spatel.

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.


https://reviews.llvm.org/D55459

Files:
  lib/CodeGen/SelectionDAG/DAGCombiner.cpp
  lib/Target/X86/X86ISelLowering.cpp


Index: lib/Target/X86/X86ISelLowering.cpp
===================================================================
--- lib/Target/X86/X86ISelLowering.cpp
+++ lib/Target/X86/X86ISelLowering.cpp
@@ -39130,6 +39130,14 @@
   EVT InVT = N0.getValueType();
   EVT InSVT = InVT.getScalarType();
 
+  // 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.
+  // FIXME: Can we remove this?
+  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();
Index: lib/CodeGen/SelectionDAG/DAGCombiner.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -8609,6 +8609,10 @@
       // if this is the case.
       EVT SVT = getSetCCResultType(N00VT);
 
+      // If we already have the desired type, don't change it.
+      if (SVT == N0.getValueType())
+        return SDValue();
+
       // 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,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D55459.177305.patch
Type: text/x-patch
Size: 1485 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181207/77fafaf8/attachment.bin>


More information about the llvm-commits mailing list