[AVX512] lowerV4X128VectorShuffle doesn't correctly select input operands .

[igorb]

Summary

[AVX512] lowerV4X128VectorShuffle doesn't correctly select input operands .
https://llvm.org/bugs/show_bug.cgi?id=26914

[RKSimon]

lib/Target/X86/X86ISelLowering.cpp

11562 ↗

(On Diff #55809)

Why not just set the Ops[] element to UNDEF?

test/CodeGen/X86/vector-shuffle-512-v8.ll

2283 ↗

(On Diff #55809)

Typically we just use a single 'u' to indicate that the shuffle mask element is undefined. In this case the test would be called: shuffle_v8f64_2301uu67. Similar for the other tests.

[igorb]

Changed according to comments,
Thanks!

[RKSimon]

LGTM

test/CodeGen/X86/vector-shuffle-512-v8.ll

2299 ↗

(On Diff #55817)

Shouldn't this be shuffle_v8f64_uuuu2301 ?

[igorb]

Thanks for review!

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

static SDValue lowerV4X128VectorShuffle(SDLoc DL, MVT VT,
   // To handle 256 bit vector requires VLX and most probably
   // function lowerV2X128VectorShuffle() is better solution.
   assert(VT.is512BitVector() && "Unexpected vector size for 128bit shuffle.");
 
   SmallVector<int, 4> WidenedMask;
   if (!canWidenShuffleElements(Mask, WidenedMask))
     return SDValue();
 
+  SDValue Ops[2] = {DAG.getUNDEF(VT), DAG.getUNDEF(VT)};
+  // Insure elements came from the same Op.
+  int MaxOp1Index = VT.getVectorNumElements()/2 - 1;
+  for (int i = 0, Size = WidenedMask.size(); i < Size; ++i) {
+    if (WidenedMask[i] == SM_SentinelZero)
+      return SDValue();
+    if (WidenedMask[i] == SM_SentinelUndef)
+      continue;
+
+    SDValue Op = WidenedMask[i] > MaxOp1Index ? V2 : V1;
+    unsigned OpIndex = (i < Size/2) ? 0 : 1;
+    if (Ops[OpIndex].isUndef())
+      Ops[OpIndex] = Op;
+    else if (Ops[OpIndex] != Op)
+      return SDValue();
+  }
+
   // Form a 128-bit permutation.
   // Convert the 64-bit shuffle mask selection values into 128-bit selection
   // bits defined by a vshuf64x2 instruction's immediate control byte.
   unsigned PermMask = 0, Imm = 0;
   unsigned ControlBitsNum = WidenedMask.size() / 2;
 
   for (int i = 0, Size = WidenedMask.size(); i < Size; ++i) {
-    if (WidenedMask[i] == SM_SentinelZero)
-      return SDValue();
-
     // Use first element in place of undef mask.
     Imm = (WidenedMask[i] == SM_SentinelUndef) ? 0 : WidenedMask[i];
     PermMask |= (Imm % WidenedMask.size()) << (i * ControlBitsNum);
   }
 
-  return DAG.getNode(X86ISD::SHUF128, DL, VT, V1, V2,
+  return DAG.getNode(X86ISD::SHUF128, DL, VT, Ops[0], Ops[1],
                      DAG.getConstant(PermMask, DL, MVT::i8));
 }
 
 static SDValue lowerVectorShuffleWithPERMV(SDLoc DL, MVT VT,
                                            ArrayRef<int> Mask, SDValue V1,
                                            SDValue V2, SelectionDAG &DAG) {
 
   assert(VT.getScalarSizeInBits() >= 16 && "Unexpected data type for PERMV");

llvm/trunk/test/CodeGen/X86/vector-shuffle-512-v8.ll

 ;
 ; AVX512F-32-LABEL: test_vshuff32x4_512:
 ; AVX512F-32:       # BB#0:
 ; AVX512F-32-NEXT:    vshuff64x2 {{.*#+}} zmm0 = zmm0[0,1,2,3],zmm1[2,3,0,1]
 ; AVX512F-32-NEXT:    retl
   %res = shufflevector <16 x float> %x, <16 x float> %x1, <16 x i32> <i32 0, i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7, i32 20, i32 21, i32 22, i32 23, i32 16, i32 17, i32 18, i32 19>
   ret <16 x float> %res
 }
+
+define <8 x double> @shuffle_v8f64_23014567(<8 x double> %a0, <8 x double> %a1) {
+; ALL-LABEL: shuffle_v8f64_23014567:
+; ALL:       # BB#0:
+; ALL-NEXT:    vshuff64x2 $225, %zmm1, %zmm1, %zmm0 # zmm0 = zmm1[2,3,0,1,4,5,6,7]
+  %1 = shufflevector <8 x double> %a1, <8 x double> undef, <8 x i32> <i32 2, i32 3, i32 0, i32 1, i32 4, i32 5, i32 6, i32 7>
+  ret <8 x double> %1
+}
+
+define <8 x double> @shuffle_v8f64_2301uu67(<8 x double> %a0, <8 x double> %a1) {
+; ALL-LABEL: shuffle_v8f64_2301uu67:
+; ALL:       # BB#0:
+; ALL-NEXT:    vshuff64x2 $193, %zmm1, %zmm1, %zmm0 # zmm0 = zmm1[2,3,0,1,0,1,6,7]
+  %1 = shufflevector <8 x double> %a1, <8 x double> undef, <8 x i32> <i32 2, i32 3, i32 0, i32 1, i32 undef, i32 undef, i32 6, i32 7>
+  ret <8 x double> %1
+}
+
+define <8 x double> @shuffle_v8f64_2301uuuu(<8 x double> %a0, <8 x double> %a1) {
+; ALL-LABEL: shuffle_v8f64_2301uuuu:
+; ALL:       # BB#0:
+; ALL-NEXT:    vshuff64x2 $1, %zmm0, %zmm1, %zmm0 # zmm0 = zmm1[2,3,0,1],zmm0[0,1,0,1]
+  %1 = shufflevector <8 x double> %a1, <8 x double> undef, <8 x i32> <i32 2, i32 3, i32 0, i32 1, i32 undef, i32 undef, i32 undef, i32 undef>
+  ret <8 x double> %1
+}
+
+define <8 x double> @shuffle_v8f64_uuu2301(<8 x double> %a0, <8 x double> %a1) {
+; ALL-LABEL: shuffle_v8f64_uuu2301:
+; ALL:       # BB#0:
+; ALL-NEXT:    vshuff64x2 $16, %zmm1, %zmm0, %zmm0 # zmm0 = zmm0[0,1,0,1],zmm1[2,3,0,1]
+  %1 = shufflevector <8 x double> %a1, <8 x double> undef, <8 x i32> <i32 undef, i32 undef, i32 undef, i32 undef, i32 2, i32 3, i32 0, i32 1>
+  ret <8 x double> %1
+}