[PATCH] D24491: [DAG] Allow build_vector-to-vector_shuffle combine to combine builds from two overly-wide vectors.

[Andrea Di Biagio via llvm-commits]

andreadb accepted this revision.
andreadb added a comment.
This revision is now accepted and ready to land.

Hi Michael,

The patch looks good to me . I only have one comment (see below).

-Andrea


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12962-12973
@@ -12957,11 +12961,14 @@
 
   // We can't generate a shuffle node with mismatched input and output types.
   // Try to make the types match.
   if (InVT1 != VT || InVT2 != VT) {
     // Both inputs and the output must have the same base element type.
     EVT ElemType = VT.getVectorElementType();
     if (ElemType != InVT1.getVectorElementType() ||
         ElemType != InVT2.getVectorElementType())
       return SDValue();
 
+    // TODO: Canonicalize this so that if the vectors have different lengths,
+    // VecIn1 is always longer.
+
     // The element types match, now figure out the lengths.
----------------
My understanding is that we check if VT is a legal type for the target at the very beginning of this method.

However, (correct me if I am wrong) we don't check if InVT1 and InVT2 are legal types. So, we potentially always enable this canonicalization even on illegal vector types.

I wonder if we should add a early exit for the case where the DAG is not "type legalized" and InVT1 and InVT2 are not legal types for the target. The legalizer would adjust/canonicalize those vector types. We would still end up running your code if eventually types still don't match VT.

Not sure if this makes sense.


https://reviews.llvm.org/D24491