[PATCH] D24683: [DAGCombine] Generalize build_vector -> vector_shuffle combine for more than 2 inputs

Michael Kuperstein via llvm-commits llvm-commits at lists.llvm.org
Wed Sep 28 02:53:56 PDT 2016


mkuper added inline comments.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13030
@@ +13029,3 @@
+    // All inputs must have the same element type as the output.
+    if (VT.getVectorElementType() !=
+        ExtractedFromVec.getValueType().getVectorElementType())
----------------
delena wrote:
> mkuper wrote:
> > delena wrote:
> > > I suppose you can put "assert" here.
> > I guess the comment is ambiguous - by "must" I didn't mean that we never get here if that doesn't happen, but that it's required for the combine. I'll change the comment.
> You can't create BUILD_VECTOR node of v8i32 and put i8 operands for it.
Yes, that's why I check that, and bail out if the types don't match. But this is the first place that actually checks this (I think?), so this has to be a failure return, not an assert.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13038
@@ +13037,3 @@
+    unsigned Idx = std::distance(
+        VecIn.begin(), std::find(VecIn.begin(), VecIn.end(), ExtractedFromVec));
+    if (Idx == VecIn.size())
----------------
delena wrote:
> line alignment
Will fix, thanks.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13045
@@ +13044,3 @@
+
+  // If we didn't find at least one input vector, bail out.
+  if (VecIn.size() < 2)
----------------
delena wrote:
> at least two?
VecIn[0] is reserved for the zero vector. So it's really at least one.
(We actually care about the case where it's exactly 1 - regardless of whether we're blending with 0 or not.)

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13073
@@ +13072,3 @@
+    SDValue VecRight =
+        (LeftIdx + 1) < VecIn.size() ? VecIn[LeftIdx + 1] : SDValue();
+
----------------
delena wrote:
> line alignment
Will fix, thanks.
I was sure I ran clang-format over this...


https://reviews.llvm.org/D24683





More information about the llvm-commits mailing list