Canonicalize splats as build_vectors (PR22283)

[spatel]

Summary

This is a minimal follow-on patch to:
http://reviews.llvm.org/D7093

That patch canonicalized constant splats as build_vectors, and this patch just removes the constant check so we can canonicalize all splats as build_vectors.

This fixes the 2nd test case in PR22283 ( http://llvm.org/bugs/show_bug.cgi?id=22283 ).

The unfortunate code duplication between SelectionDAG and DAGCombiner is discussed in the earlier patch. At least this patch is just removing code...

This improves an existing x86 AVX test and changes codegen in an ARM test. I don't know enough about ARM implementations to know if the new codegen is an improvement or not.

For the changed ARM tests, we currently generate something like this:

ldrsh	r12, [sp]   <--- load scalar
vmov	d17, r2, r3
vmov	d16, r0, r1
vmov	s0, r12
vcvt.f32.s32	s0, s0
vdup.32	q9, d0[0]     <--- splat register value
vsub.f32	q8, q9, q8
vmov	r0, r1, d16
vmov	r2, r3, d17
mov	pc, lr

With this patch, we'll generate a 'vld1' to splat instead:

mov	r12, sp
vmov	d19, r2, r3
vld1.16	{d16[]}, [r12:16] <--- load and splat
vmov	d18, r0, r1
vmovl.s16	q8, d16
vcvt.f32.s32	q8, q8
vsub.f32	q8, q8, q9
vmov	r0, r1, d16
vmov	r2, r3, d17
mov	pc, lr

[mkuper]

From the x86 point of view, LGTM with a small nit.
Can't help you with ARM code quality, though.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

1563 ↗

(On Diff #19261)

There's a missing space after the =.

[spatel]

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

1563 ↗

(On Diff #19261)

That was an 80-column hack...so close. :)

[andreadb]

Hi Sanjay,

the patch LGTM.
However, somebody should review the codegen change in the ARM test.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

1563 ↗

(On Diff #19261)

There are two places where we check for the BUILD_VECTOR value type. One is here at line 1563, the other one is at line 1567. If you evaluate 'BV->getValueType(0)' before the call to 'getNode' (line 1563), then you can reuse it and it will fit in the 80-cols.

[spatel]

Thanks, Andrea. Minor update to use local VT var; patch rebased against r228149.

[spatel]

Adding Anton as potential reviewer of the ARM change.

[rengolin]

Adding James, as he knows a lot more about NEON than I do.

[spatel]

Added the missing space...really this time.

[spatel]

Ping.

ARM experts: is 'vld1' any better / worse than 'vdup' here?

[jmolloy]

LGTM - splatting ld1 is better than ldrsh/vdup (at least from an architectural standpoint).

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (V->getOpcode() == ISD::BUILD_VECTOR) {
           AllSame = false;
           break;
         }
       }
       // Splat of <x, x, x, x>, return <x, x, x, x>
       if (AllSame)
         return N0;
 
-      // If the splatted element is a constant, just build the vector out of
-      // constants directly.
+      // Canonicalize any other splat as a build_vector.
       const SDValue &Splatted = V->getOperand(SVN->getSplatIndex());
-      if (isa<ConstantSDNode>(Splatted) || isa<ConstantFPSDNode>(Splatted)) {
-        SmallVector<SDValue, 8> Ops(NumElts, Splatted);
-        SDValue NewBV = DAG.getNode(ISD::BUILD_VECTOR, SDLoc(N),
                                   V->getValueType(0), Ops);
 
-        // We may have jumped through bitcasts, so the type of the
-        // BUILD_VECTOR may not match the type of the shuffle.
-        if (V->getValueType(0) != VT)
-           NewBV = DAG.getNode(ISD::BITCAST, SDLoc(N), VT, NewBV);
-        return NewBV;
-      }
-    }
-  }
 
   // There are various patterns used to build up a vector from smaller vectors,
   // subvectors, or elements. Scan chains of these and replace unused insertions
   // or components with undef.
   if (SDValue S = simplifyShuffleOperands(SVN, N0, N1, DAG))
     return S;
 
   if (N0.getOpcode() == ISD::CONCAT_VECTORS &&

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

if (auto *BV = dyn_cast<BuildVectorSDNode>(V)) {
         // number of elements match or the value splatted is a zero constant.
         if (SameNumElts)
           return N1;
         if (auto *C = dyn_cast<ConstantSDNode>(Splat))
           if (C->isNullValue())
             return N1;
       }
 
-      // If the shuffle itself creates a constant splat, build the vector
-      // directly.
+      // If the shuffle itself creates a splat, build the vector directly.
       if (AllSame && SameNumElts) {
-         const SDValue &Splatted = BV->getOperand(MaskVec[0]);
-         if (isa<ConstantSDNode>(Splatted) || isa<ConstantFPSDNode>(Splatted)) {
-           SmallVector<SDValue, 8> Ops(NElts, Splatted);
 
-           SDValue NewBV =
-               getNode(ISD::BUILD_VECTOR, dl, BV->getValueType(0), Ops);
+        EVT BuildVT = BV->getValueType(0);
+        SDValue NewBV = getNode(ISD::BUILD_VECTOR, dl, BuildVT, Ops);
 
-           // We may have jumped through bitcasts, so the type of the
-           // BUILD_VECTOR may not match the type of the shuffle.
-           if (BV->getValueType(0) != VT)
-             NewBV = getNode(ISD::BITCAST, dl, VT, NewBV);
-           return NewBV;
-         }
-      }
-    }
-  }
+        if (BuildVT != VT)
 
   FoldingSetNodeID ID;
   SDValue Ops[2] = { N1, N2 };
   AddNodeIDNode(ID, ISD::VECTOR_SHUFFLE, getVTList(VT), Ops);
   for (unsigned i = 0; i != NElts; ++i)
     ID.AddInteger(MaskVec[i]);
 
   void* IP = nullptr;

llvm/trunk/test/CodeGen/ARM/vdup.ll

;CHECK: vdup.32 d
   %splat.splatinsert = insertelement <2 x float> undef, float %conv, i32 0
   %splat.splat = shufflevector <2 x float> %splat.splatinsert, <2 x float> undef, <2 x i32> zeroinitializer
   %sub = fsub <2 x float> %splat.splat, %p
   ret <2 x float> %sub
 }
 
 define <4 x float> @check_spr_splat4(<4 x float> %p, i16 %q) {
 ;CHECK-LABEL: check_spr_splat4:
-;CHECK: vdup.32 q
+;CHECK: vld1.16
   %conv = sitofp i16 %q to float
   %splat.splatinsert = insertelement <4 x float> undef, float %conv, i32 0
   %splat.splat = shufflevector <4 x float> %splat.splatinsert, <4 x float> undef, <4 x i32> zeroinitializer
   %sub = fsub <4 x float> %splat.splat, %p
   ret <4 x float> %sub
 }
-
+; Same codegen as above test; scalar is splatted using vld1, so shuffle index is irrelevant.
 define <4 x float> @check_spr_splat4_lane1(<4 x float> %p, i16 %q) {
 ;CHECK-LABEL: check_spr_splat4_lane1:
-;CHECK: vdup.32 q{{.*}}, d{{.*}}[1]
+;CHECK: vld1.16
   %conv = sitofp i16 %q to float
   %splat.splatinsert = insertelement <4 x float> undef, float %conv, i32 1
   %splat.splat = shufflevector <4 x float> %splat.splatinsert, <4 x float> undef, <4 x i32> <i32 1, i32 1, i32 1, i32 1>
   %sub = fsub <4 x float> %splat.splat, %p
   ret <4 x float> %sub
 }

llvm/trunk/test/CodeGen/X86/vector-shuffle-256-v4.ll

; ALL-NEXT:    retq
   %v = insertelement <4 x double> undef, double %a, i32 0
   %shuffle = shufflevector <4 x double> %v, <4 x double> undef, <4 x i32> <i32 0, i32 0, i32 0, i32 0>
   ret <4 x double> %shuffle
 }
 
 define <4 x i64> @splat_mem_v4i64(i64* %ptr) {
 ; AVX1-LABEL: splat_mem_v4i64:
 ; AVX1:       # BB#0:
-; AVX1-NEXT:    vmovddup {{.*#+}} xmm0 = mem[0,0]
-; AVX1-NEXT:    vinsertf128 $1, %xmm0, %ymm0, %ymm0
+; AVX1-NEXT:    vbroadcastsd (%rdi), %ymm0
 ; AVX1-NEXT:    retq
 ;
 ; AVX2-LABEL: splat_mem_v4i64:
 ; AVX2:       # BB#0:
 ; AVX2-NEXT:    vbroadcastsd (%rdi), %ymm0
 ; AVX2-NEXT:    retq
   %a = load i64* %ptr
   %v = insertelement <4 x i64> undef, i64 %a, i64 0