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

[Simon Pilgrim via llvm-commits]

RKSimon added a comment.

Thanks for looking at this! I've made a few minor observations.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12996
@@ +12995,3 @@
+        // output, split it in two.        
+        VecIn2 = DAG.getNode(ISD::EXTRACT_SUBVECTOR, dl, VT, VecIn1,
+                             DAG.getConstant(NumElems, dl, IdxTy));
----------------
Its not necessarily twice the size here is it? I'm thinking xmm from zmm.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13054-13055
@@ +13053,4 @@
+  // only care about the low elements anyway. Pad the mask with undef.
+  for (unsigned i = NumElems; i != ShuffleNumElems; ++i)
+    Mask.push_back(-1);
+
----------------
mkuper wrote:
> ABataev wrote:
> > ```
> > if (ShuffleNumElems > NumElems)
> >   Mask.append(ShuffleNumElems-NumElems, -1)
> > ```
> > ?
> Yeah, I keep writing these loops on auto-pilot. :-)
> This should either be an append, or Mask should just be initialized to -1. Thanks!
It'd be tidier if you just did SmallVector<int, 8> Mask(ShuffleNumElems, -1) and replaced the push_back() calls with Mask[i]

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13079
@@ +13078,3 @@
+    Shuffle = DAG.getNode(ISD::EXTRACT_SUBVECTOR, dl, VT, Shuffle,
+                          DAG.getConstant(0, dl, IdxTy));
+
----------------
Use ZeroIdx?


https://reviews.llvm.org/D24491