[DAGCombine] Improve handling of insert_subvector of bitcast values

[niravd]

Summary

Add variations on existing optimizations that deal with bitcasts in operands. Fixes regressions from X86's post-legalization store merge change.

[RKSimon]

Waiting on test cases.

[craig.topper]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

15473 ↗

(On Diff #103767)

"If we N0"?

15485 ↗

(On Diff #103767)

Period on the end of the comment.

15494 ↗

(On Diff #103767)

No space between "bit" and "cast" to match the comment above?

[niravd]

It turns out generating the failing case for this nontrivial as it looks like we canot irectly build a insertsubvector /extract subvector node.

The peephole of note is along the lines of:

insertsubvector (insertsubvector undef (extractsubvector (bitcast x) 0) 0) (extract subvector (bitcast x) (subvectorsize)) subvectorsize

becoming :

(bitcast x)

Rather than push on this here, I'm going to flip the dependence with D34559 which will now have a small degradation that this patch will fix.

[niravd]

Comment cleanup

[niravd]

Rebase and move after X86 post-legalization store merge to provide test cases.

[RKSimon]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

15525 ↗

(On Diff #104325)

N0.getOpcode()

[niravd]

Resolve comments.

[niravd]

Unfold unrelated commit.

[niravd]

Undo unrelated updated.

[RKSimon]

LGTM

[RKSimon]

@niravd Anything stopping this going in?

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

SDValue DAGCombiner::visitINSERT_SUBVECTOR(SDNode *N) {
   SDValue N0 = N->getOperand(0);
   SDValue N1 = N->getOperand(1);
   SDValue N2 = N->getOperand(2);
 
   // If inserting an UNDEF, just return the original vector.
   if (N1.isUndef())
     return N0;
 
+  // For nested INSERT_SUBVECTORs, attempt to combine inner node first to allow
+  // us to pull BITCASTs from input to output.
+  if (N0.hasOneUse() && N0->getOpcode() == ISD::INSERT_SUBVECTOR)
+    if (SDValue NN0 = visitINSERT_SUBVECTOR(N0.getNode()))
+      return DAG.getNode(ISD::INSERT_SUBVECTOR, SDLoc(N), VT, NN0, N1, N2);
+
   // If this is an insert of an extracted vector into an undef vector, we can
   // just use the input to the extract.
   if (N0.isUndef() && N1.getOpcode() == ISD::EXTRACT_SUBVECTOR &&
       N1.getOperand(1) == N2 && N1.getOperand(0).getValueType() == VT)
     return N1.getOperand(0);
 
+  // If we are inserting a bitcast value into an undef, with the same
+  // number of elements, just use the bitcast input of the extract.
+  // i.e. INSERT_SUBVECTOR UNDEF (BITCAST N1) N2 ->
+  //        BITCAST (INSERT_SUBVECTOR UNDEF N1 N2)
+  if (N0.isUndef() && N1.getOpcode() == ISD::BITCAST &&
+      N1.getOperand(0).getOpcode() == ISD::EXTRACT_SUBVECTOR &&
+      N1.getOperand(0).getOperand(1) == N2 &&
+      N1.getOperand(0).getOperand(0).getValueType().getVectorNumElements() ==
+          VT.getVectorNumElements()) {
+    return DAG.getBitcast(VT, N1.getOperand(0).getOperand(0));
+  }
+
+  // If both N1 and N2 are bitcast values on which insert_subvector
+  // would makes sense, pull the bitcast through.
+  // i.e. INSERT_SUBVECTOR (BITCAST N0) (BITCAST N1) N2 ->
+  //        BITCAST (INSERT_SUBVECTOR N0 N1 N2)
+  if (N0.getOpcode() == ISD::BITCAST && N1.getOpcode() == ISD::BITCAST) {
+    SDValue CN0 = N0.getOperand(0);
+    SDValue CN1 = N1.getOperand(0);
+    if (CN0.getValueType().getVectorElementType() ==
+            CN1.getValueType().getVectorElementType() &&
+        CN0.getValueType().getVectorNumElements() ==
+            VT.getVectorNumElements()) {
+      SDValue NewINSERT = DAG.getNode(ISD::INSERT_SUBVECTOR, SDLoc(N),
+                                      CN0.getValueType(), CN0, CN1, N2);
+      return DAG.getBitcast(VT, NewINSERT);
+    }
+  }
+
   // Combine INSERT_SUBVECTORs where we are inserting to the same index.
   // INSERT_SUBVECTOR( INSERT_SUBVECTOR( Vec, SubOld, Idx ), SubNew, Idx )
   // --> INSERT_SUBVECTOR( Vec, SubNew, Idx )
   if (N0.getOpcode() == ISD::INSERT_SUBVECTOR &&
       N0.getOperand(1).getValueType() == N1.getValueType() &&
       N0.getOperand(2) == N2)
     return DAG.getNode(ISD::INSERT_SUBVECTOR, SDLoc(N), VT, N0.getOperand(0),
                        N1, N2);

llvm/trunk/test/CodeGen/X86/MergeConsecutiveStores.ll

define void @merge_vec_element_store(<8 x float> %v, float* %ptr) {
   store float %vecext2, float* %arrayidx2, align 4
   store float %vecext3, float* %arrayidx3, align 4
   store float %vecext4, float* %arrayidx4, align 4
   store float %vecext5, float* %arrayidx5, align 4
   store float %vecext6, float* %arrayidx6, align 4
   store float %vecext7, float* %arrayidx7, align 4
   ret void
 
-; CHECK: vextractf128 $1, %ymm0, %xmm1
-; CHECK: vinsertf128  $1, %xmm1, %ymm0, %ymm0
+; CHECK-LABEL: merge_vec_element_store
+; CHECK: vmovups %ymm0, (%rdi)
+; CHECK: vzeroupper
 ; CHECK: retq
-
-; This is what should be generated:
-; FIXME-LABEL: merge_vec_element_store
-; FIXME: vmovups
-; FIXME-NEXT: vzeroupper
-; FIXME-NEXT: retq
 }
 
 ; PR21711 - Merge vector stores into wider vector stores.
 ; These should be merged into 32-byte stores.
 define void @merge_vec_extract_stores(<8 x float> %v1, <8 x float> %v2, <4 x float>* %ptr) {
   %idx0 = getelementptr inbounds <4 x float>, <4 x float>* %ptr, i64 3
   %idx1 = getelementptr inbounds <4 x float>, <4 x float>* %ptr, i64 4
   %idx2 = getelementptr inbounds <4 x float>, <4 x float>* %ptr, i64 5
   %idx3 = getelementptr inbounds <4 x float>, <4 x float>* %ptr, i64 6
   %shuffle0 = shufflevector <8 x float> %v1, <8 x float> undef, <4 x i32> <i32 0, i32 1, i32 2, i32 3>
   %shuffle1 = shufflevector <8 x float> %v1, <8 x float> undef, <4 x i32> <i32 4, i32 5, i32 6, i32 7>
   %shuffle2 = shufflevector <8 x float> %v2, <8 x float> undef, <4 x i32> <i32 0, i32 1, i32 2, i32 3>
   %shuffle3 = shufflevector <8 x float> %v2, <8 x float> undef, <4 x i32> <i32 4, i32 5, i32 6, i32 7>
   store <4 x float> %shuffle0, <4 x float>* %idx0, align 16
   store <4 x float> %shuffle1, <4 x float>* %idx1, align 16
   store <4 x float> %shuffle2, <4 x float>* %idx2, align 16
   store <4 x float> %shuffle3, <4 x float>* %idx3, align 16
   ret void
 
-; These vblendpd are obviously redundant.
-; CHECK: vblendpd $12, %ymm0, %ymm0, %ymm0 # ymm0 = ymm0[0,1,2,3]
-; CHECK: vmovupd  %ymm0, 48(%rdi)
-; CHECK: vblendpd $12, %ymm1, %ymm1, %ymm0 # ymm0 = ymm1[0,1,2,3]
-; CHECK: vmovupd  %ymm0, 80(%rdi)
-
-; This is what should be generated:
-; FIXME-LABEL: merge_vec_extract_stores
-; FIXME:      vmovups %ymm0, 48(%rdi)
-; FIXME-NEXT: vmovups %ymm1, 80(%rdi)
-; FIXME-NEXT: vzeroupper
-; FIXME-NEXT: retq
+; CHECK-LABEL: merge_vec_extract_stores
+; CHECK:      vmovups %ymm0, 48(%rdi)
+; CHECK-NEXT: vmovups %ymm1, 80(%rdi)
+; CHECK-NEXT: vzeroupper
+; CHECK-NEXT: retq
 }
 
 ; Merging vector stores when sourced from vector loads.
 define void @merge_vec_stores_from_loads(<4 x float>* %v, <4 x float>* %ptr) {
   %load_idx0 = getelementptr inbounds <4 x float>, <4 x float>* %v, i64 0
   %load_idx1 = getelementptr inbounds <4 x float>, <4 x float>* %v, i64 1
   %v0 = load <4 x float>, <4 x float>* %load_idx0
   %v1 = load <4 x float>, <4 x float>* %load_idx1

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

 ; AVX512VL-NEXT:    retq
   %shuffle = shufflevector <4 x i64> %a, <4 x i64> %b, <4 x i32> <i32 0, i32 1, i32 4, i32 2>
   ret <4 x i64> %shuffle
 }
 
 define <4 x i64> @shuffle_v4i64_0412(<4 x i64> %a, <4 x i64> %b) {
 ; AVX1-LABEL: shuffle_v4i64_0412:
 ; AVX1:       # BB#0:
+; AVX1-NEXT:    vmovddup {{.*#+}} xmm1 = xmm1[0,0]
 ; AVX1-NEXT:    vextractf128 $1, %ymm0, %xmm2
 ; AVX1-NEXT:    vpalignr {{.*#+}} xmm2 = xmm0[8,9,10,11,12,13,14,15],xmm2[0,1,2,3,4,5,6,7]
 ; AVX1-NEXT:    vinsertf128 $1, %xmm2, %ymm0, %ymm0
-; AVX1-NEXT:    vmovddup {{.*#+}} xmm1 = xmm1[0,0]
 ; AVX1-NEXT:    vblendpd {{.*#+}} ymm0 = ymm0[0],ymm1[1],ymm0[2,3]
 ; AVX1-NEXT:    retq
 ;
 ; AVX2-LABEL: shuffle_v4i64_0412:
 ; AVX2:       # BB#0:
 ; AVX2-NEXT:    vpbroadcastq %xmm1, %xmm1
 ; AVX2-NEXT:    vpermq {{.*#+}} ymm0 = ymm0[0,1,1,2]
 ; AVX2-NEXT:    vpblendd {{.*#+}} ymm0 = ymm0[0,1],ymm1[2,3],ymm0[4,5,6,7]