[PATCH] [DAGCombiner] Combine shuffles of BUILD_VECTOR and SCALAR_TO_VECTOR

[Simon Pilgrim]

Thanks Andrea. Are you happy with me submitting with these changes (after tests) or would prefer another review?


REPOSITORY
  rL LLVM

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12033-12035
@@ +12032,5 @@
+    if (Ops.size() == VT.getVectorNumElements()) {
+      // Create SCALAR_TO_VECTOR if the only defined input is input[0].
+      if (1 == NumDefElts && Ops[0].getOpcode() != ISD::UNDEF)
+        return DAG.getNode(ISD::SCALAR_TO_VECTOR, SDLoc(N), VT, Ops[0]);
+
----------------
andreadb wrote:
> On x86, a build_vector with all operands undef excluding the first operand is legalized to a scalar_to_vector. I am not sure if other targets would do the same; in case, I would suggest to remove this code and just emit a build_vector.
> Do you have an example that relies on this check? A shuffles with only one non-undef element should have been canonicalized/simplified to a shuffle where one of the operands is undef.
No specific need - its easy enough to always create a BUILD_VECTOR and rely on a target's lowering logic to do the right thing.

================
Comment at: test/CodeGen/X86/mmx-bitcast.ll:78
@@ +77,3 @@
+; CHECK-NEXT:    punpckldq {{.*#+}} xmm1 = xmm1[0],xmm0[0],xmm1[1],xmm0[1]
+; CHECK-NEXT:    movd %xmm1, %rax
+; CHECK-NEXT:    retq
----------------
andreadb wrote:
> I don't think this is related to your patch. However, this looks like a bug to me. Shouldn't this be a 'movq'?
movd deals with 32 and 64-bit gprs <-> vector moves. movq does vector load/stores and vector <-> vector moves. Its a funny old world.

================
Comment at: test/CodeGen/X86/sse41.ll:1028-1029
@@ -1027,4 @@
-
-; Edge case for insertps where we end up with a shuffle with mask=<0, 7, -1, -1>
-define void @insertps_pr20411(i32* noalias nocapture %RET) #1 {
-; X32-LABEL: insertps_pr20411:
----------------
andreadb wrote:
> My question is: do we have an equivalent test for insertps somewhere else? If so, then I think it is OK to remove it. Otherwise I would keep it.
Its a reduced test that with that patch folds to a store of a constant. I'll change it to use non-constant vector data and see if that works.

================
Comment at: test/CodeGen/X86/vector-shuffle-128-v16.ll:646
@@ +645,3 @@
+; AVX-NEXT:    movzbl %dil, %eax
+; AVX-NEXT:    movd %eax, %xmm0
+; AVX-NEXT:    retq
----------------
andreadb wrote:
> Shouldn't this be 'vmovd'?
Fixed.

http://reviews.llvm.org/D8516

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/