[X86] Don't use known bits to simplify shuffles.

[efriedma]

Summary

This reverts commit 2a419a0b9957ebac9e11e4b43bc9fbe42a9207df.

The result of a shufflevector must not propagate poison from any element other than the one noted in the shuffle mask.

The regressions outside of fptoui-may-overflow.ll can probably be recovered some other way.

See https://reviews.llvm.org/D106053 for more background.

[xiangzhangllvm]

The result of a shufflevector must not propagate poison from any element other than the one noted in the shuffle mask.
However, this means the 2a419a0b transform isn't safe: if a lane in the result of a shufflevector is supposed to be zero, it has to actually be zero, not zero-or-poison.

Sorry, I still not much understand, where we got the poison value in following transformation.
computeKnownBits will only meet "AssertZext %f i8", how does it know there will be poison value ?

%f = fptoui <4 x float> %arg to <4 x i8> -->   fptosi <4 x float> %1 to <4 x i16> + AssertZext  %f,  i8

[RKSimon]

In D106222#2885780, @xiangzhangllvm wrote:

The result of a shufflevector must not propagate poison from any element other than the one noted in the shuffle mask.
However, this means the 2a419a0b transform isn't safe: if a lane in the result of a shufflevector is supposed to be zero, it has to actually be zero, not zero-or-poison.

Sorry, I still not much understand, where we got the poison value in following transformation.
computeKnownBits will only meet "AssertZext %f i8", how does it know there will be poison value ?

%f = fptoui <4 x float> %arg to <4 x i8> -->   fptosi <4 x float> %1 to <4 x i16> + AssertZext  %f,  i8

It doesn't know there will be poison, it just knows that there COULD be poison.

Unless there's a way to prove that the source float data is 0.0f<x<256.0f then we have to fail safe and assume it could contain poison under some circumstances.

I think we're going to have to add something to the SelectionDAG::computeKnownBits/ComputeNumSignBits doxygen comments explaining that the value could have the determined known/signbits BUT it could be a poison value (or if a vector any demanded element could be poison value).

[efriedma]

In D106222#2885959, @RKSimon wrote:

I think we're going to have to add something to the SelectionDAG::computeKnownBits/ComputeNumSignBits doxygen comments explaining that the value could have the determined known/signbits BUT it could be a poison value (or if a vector any demanded element could be poison value).

Sure, probably all these functions could use a comment like that: computeKnownBits, ComputeNumSignBits, haveNoCommonBitsSet, isKnownToBeAPowerOfTwo, computeOverflowKind, MaskedValueIsAllOnes, MaskedValueIsZero, SignBitIsZero, isKnownNeverNaN, isSplatValue, etc. And probably the ValueTracking equivalents, too. (Some of the ValueTracking functions already have comments which subtly indicate this, e.g. "exactly one bit set when defined.")

[RKSimon]

OK, I'll put together a patch updating the comments.

I think this patch gets to the real problem (although there might be other similar issues) and helps us avoid the avoidable nasty regressions from D106053 - does anyone else have any thoughts?

[aqjune]

In D106222#2886166, @efriedma wrote:

In D106222#2885959, @RKSimon wrote:

I think we're going to have to add something to the SelectionDAG::computeKnownBits/ComputeNumSignBits doxygen comments explaining that the value could have the determined known/signbits BUT it could be a poison value (or if a vector any demanded element could be poison value).

Sure, probably all these functions could use a comment like that: computeKnownBits, ComputeNumSignBits, haveNoCommonBitsSet, isKnownToBeAPowerOfTwo, computeOverflowKind, MaskedValueIsAllOnes, MaskedValueIsZero, SignBitIsZero, isKnownNeverNaN, isSplatValue, etc. And probably the ValueTracking equivalents, too. (Some of the ValueTracking functions already have comments which subtly indicate this, e.g. "exactly one bit set when defined.")

+1, this clarification looks great to me.

[xiangzhangllvm]

I wish we can first commit this patch to solve our urgent project fail.

@RKSimon, Or Maybe we can replace the AssertZext with a new node AssertZextOrPoison in "May Be Poison" case ? (If it is much easier to change all the existed optimizations. )

[xiangzhangllvm]

Thank you very much!

llvm/include/llvm/CodeGen/SelectionDAG.h

bool MaskedValueIsZero(SDValue Op, const APInt &Mask,
                          unsigned Depth = 0) const;
 
   /// Return true if 'Op & Mask' is known to be zero in DemandedElts.  We
   /// use this predicate to simplify operations downstream.  Op and Mask are
   /// known to be the same type.
   bool MaskedValueIsZero(SDValue Op, const APInt &Mask,
                          const APInt &DemandedElts, unsigned Depth = 0) const;
 
-  /// Return true if the DemandedElts of the vector Op are all zero.  We
-  /// use this predicate to simplify operations downstream.
-  bool MaskedElementsAreZero(SDValue Op, const APInt &DemandedElts,
-                             unsigned Depth = 0) const;
-
   /// Return true if '(Op & Mask) == Mask'.
   /// Op and Mask are known to be the same type.
   bool MaskedValueIsAllOnes(SDValue Op, const APInt &Mask,
                             unsigned Depth = 0) const;
 
   /// Determine which bits of Op are known to be either zero or one and return
   /// them in Known. For vectors, the known bits are those that are shared by
   /// every vector element.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

 /// DemandedElts.  We use this predicate to simplify operations downstream.
 /// Mask is known to be zero for bits that V cannot have.
 bool SelectionDAG::MaskedValueIsZero(SDValue V, const APInt &Mask,
                                      const APInt &DemandedElts,
                                      unsigned Depth) const {
   return Mask.isSubsetOf(computeKnownBits(V, DemandedElts, Depth).Zero);
 }
 
-/// Return true if the DemandedElts of the vector Op are all zero.  We
-/// use this predicate to simplify operations downstream.
-bool SelectionDAG::MaskedElementsAreZero(SDValue Op, const APInt &DemandedElts,
-                                         unsigned Depth) const {
-  assert(Op.getValueType().isFixedLengthVector() &&
-         Op.getValueType().getVectorNumElements() ==
-             DemandedElts.getBitWidth() &&
-         "MaskedElementsAreZero vector size mismatch");
-  unsigned BitWidth = Op.getScalarValueSizeInBits();
-  APInt DemandedBits = APInt::getAllOnesValue(BitWidth);
-  return MaskedValueIsZero(Op, DemandedBits, DemandedElts, Depth);
-}
-
 /// MaskedValueIsAllOnes - Return true if '(Op & Mask) == Mask'.
 bool SelectionDAG::MaskedValueIsAllOnes(SDValue V, const APInt &Mask,
                                         unsigned Depth) const {
   return Mask.isSubsetOf(computeKnownBits(V, Depth).One);
 }
 
 /// isSplatValue - Return true if the vector V has the same value
 /// across all DemandedElts. For scalable vectors it does not make

llvm/lib/Target/X86/X86ISelLowering.cpp

if ((Subtarget.hasAVX2() ||
             return SDValue(); // Nothing to do!
           Res = CanonicalizeShuffleInput(MaskVT, V1);
           Res = DAG.getNode(X86ISD::VBROADCAST, DL, MaskVT, Res);
           return DAG.getBitcast(RootVT, Res);
         }
       }
     }
 
-    // See if this is a blend with zero - in which case check if the zero'd
-    // elements are already zero.
-    if (isSequentialOrUndefOrZeroInRange(Mask, 0, NumMaskElts, 0)) {
-      assert(!KnownZero.isNullValue() && "Shuffle has no zero elements");
-      SDValue NewV1 = CanonicalizeShuffleInput(MaskVT, V1);
-      if (DAG.MaskedElementsAreZero(NewV1, KnownZero))
-        return DAG.getBitcast(RootVT, NewV1);
-    }
-
     SDValue NewV1 = V1; // Save operand in case early exit happens.
     if (matchUnaryShuffle(MaskVT, Mask, AllowFloatDomain, AllowIntDomain, NewV1,
                           DL, DAG, Subtarget, Shuffle, ShuffleSrcVT,
                           ShuffleVT) &&
         (!IsMaskedShuffle ||
          (NumRootElts == ShuffleVT.getVectorNumElements()))) {
       if (Depth == 0 && Root.getOpcode() == Shuffle)
         return SDValue(); // Nothing to do!

llvm/test/CodeGen/X86/fptoui-may-overflow.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=avx | FileCheck %s
+
+; @fptoui_zext is legal to optimize to a single vcvttps2dq: if one of the i8
+; results of fptoui is poisoned, the corresponding i32 result of the zext is
+; also poisoned. We currently don't implement this optimization.
+
+define <16 x i8> @fptoui_zext(<4 x float> %arg) {
+; CHECK-LABEL: fptoui_zext:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    vcvttps2dq %xmm0, %xmm0
+; CHECK-NEXT:    vpackusdw %xmm0, %xmm0, %xmm0
+; CHECK-NEXT:    vpackuswb %xmm0, %xmm0, %xmm0
+; CHECK-NEXT:    vpmovzxbd {{.*#+}} xmm0 = xmm0[0],zero,zero,zero,xmm0[1],zero,zero,zero,xmm0[2],zero,zero,zero,xmm0[3],zero,zero,zero
+; CHECK-NEXT:    retq
+  %f = fptoui <4 x float> %arg to <4 x i8>
+  %z = zext <4 x i8> %f to <4 x i32>
+  %b = bitcast <4 x i32> %z to <16 x i8>
+  ret <16 x i8> %b
+}
+
+; In @fptoui_shuffle, we must preserve the vpand for correctnesss. Only the
+; i8 values extracted from %s are poison.  The values from the zeroinitializer
+; are not.
+
+define <16 x i8> @fptoui_shuffle(<4 x float> %arg) {
+; CHECK-LABEL: fptoui_shuffle:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    vcvttps2dq %xmm0, %xmm0
+; CHECK-NEXT:    vandps {{\.?LCPI[0-9]+_[0-9]+}}(%rip), %xmm0, %xmm0
+; CHECK-NEXT:    retq
+  %f = fptoui <4 x float> %arg to <4 x i8>
+  %s = shufflevector <4 x i8> %f, <4 x i8> undef, <16 x i32> <i32 0, i32 1, i32 2, i32 3, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef>
+  %ss = shufflevector <16 x i8> %s, <16 x i8> zeroinitializer, <16 x i32> <i32 0, i32 17, i32 18, i32 19, i32 1, i32 21, i32 22, i32 23, i32 2, i32 25, i32 26, i32 27, i32 3, i32 29, i32 30, i32 31>
+  ret <16 x i8> %ss
+}

llvm/test/CodeGen/X86/oddshuffles.ll

 ; SSE42-NEXT:    xorps %xmm3, %xmm3
 ; SSE42-NEXT:    retq
 ;
 ; AVX1-LABEL: splat_v3i32:
 ; AVX1:       # %bb.0:
 ; AVX1-NEXT:    vmovq {{.*#+}} xmm0 = mem[0],zero
 ; AVX1-NEXT:    vpinsrd $2, 8(%rdi), %xmm0, %xmm1
 ; AVX1-NEXT:    vxorps %xmm2, %xmm2, %xmm2
-; AVX1-NEXT:    vblendps {{.*#+}} ymm0 = ymm2[0],ymm1[1],ymm2[2,3,4,5,6,7]
-; AVX1-NEXT:    vpermilps {{.*#+}} xmm1 = xmm1[0,1,0,1]
+; AVX1-NEXT:    vblendps {{.*#+}} ymm0 = ymm2[0],ymm0[1],ymm2[2,3,4,5,6,7]
+; AVX1-NEXT:    vpshufd {{.*#+}} xmm1 = xmm1[0,1,0,1]
 ; AVX1-NEXT:    vblendps {{.*#+}} ymm1 = ymm2[0,1],ymm1[2],ymm2[3,4,5,6,7]
 ; AVX1-NEXT:    retq
 ;
 ; AVX2-SLOW-LABEL: splat_v3i32:
 ; AVX2-SLOW:       # %bb.0:
 ; AVX2-SLOW-NEXT:    vmovsd {{.*#+}} xmm1 = mem[0],zero
 ; AVX2-SLOW-NEXT:    vxorps %xmm2, %xmm2, %xmm2
 ; AVX2-SLOW-NEXT:    vblendps {{.*#+}} ymm0 = ymm2[0],ymm1[1],ymm2[2,3,4,5,6,7]

 ; AVX2-FAST-NEXT:    vpshufb {{.*#+}} ymm1 = zero,zero,zero,zero,zero,zero,zero,zero,ymm1[0,1,2,3],zero,zero,zero,zero,zero,zero,zero,zero,zero,zero,zero,zero,zero,zero,zero,zero,zero,zero,zero,zero
 ; AVX2-FAST-NEXT:    retq
 ;
 ; XOP-LABEL: splat_v3i32:
 ; XOP:       # %bb.0:
 ; XOP-NEXT:    vmovq {{.*#+}} xmm0 = mem[0],zero
 ; XOP-NEXT:    vpinsrd $2, 8(%rdi), %xmm0, %xmm1
 ; XOP-NEXT:    vxorps %xmm2, %xmm2, %xmm2
-; XOP-NEXT:    vblendps {{.*#+}} ymm0 = ymm2[0],ymm1[1],ymm2[2,3,4,5,6,7]
-; XOP-NEXT:    vpermilps {{.*#+}} xmm1 = xmm1[0,1,0,1]
+; XOP-NEXT:    vblendps {{.*#+}} ymm0 = ymm2[0],ymm0[1],ymm2[2,3,4,5,6,7]
+; XOP-NEXT:    vpshufd {{.*#+}} xmm1 = xmm1[0,1,0,1]
 ; XOP-NEXT:    vblendps {{.*#+}} ymm1 = ymm2[0,1],ymm1[2],ymm2[3,4,5,6,7]
 ; XOP-NEXT:    retq
   %1 = load <3 x i32>, <3 x i32>* %ptr, align 1
   %2 = shufflevector <3 x i32> %1, <3 x i32> undef, <16 x i32> <i32 0, i32 1, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef>
   %3 = shufflevector <16 x i32> <i32 0, i32 undef, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 undef, i32 0, i32 0, i32 0, i32 0, i32 0>, <16 x i32> %2, <16 x i32> <i32 0, i32 17, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7, i32 8, i32 9, i32 16, i32 11, i32 12, i32 13, i32 14, i32 15>
   ret <16 x i32 > %3
 }
 

llvm/test/CodeGen/X86/vector_splat-const-shift-of-constmasked.ll

 ; X86-AVX2-NEXT:    vpxor %xmm1, %xmm1, %xmm1
 ; X86-AVX2-NEXT:    vpblendd {{.*#+}} xmm0 = xmm1[0],xmm0[1],xmm1[2],xmm0[3]
 ; X86-AVX2-NEXT:    retl
 ;
 ; X64-SSE2-LABEL: test_128_i64_x_2_18446744065119617024_mask_ashr_1:
 ; X64-SSE2:       # %bb.0:
 ; X64-SSE2-NEXT:    pand {{\.?LCPI[0-9]+_[0-9]+}}(%rip), %xmm0
 ; X64-SSE2-NEXT:    psrad $1, %xmm0
+; X64-SSE2-NEXT:    pand {{\.?LCPI[0-9]+_[0-9]+}}(%rip), %xmm0
 ; X64-SSE2-NEXT:    retq
 ;
-; X64-AVX-LABEL: test_128_i64_x_2_18446744065119617024_mask_ashr_1:
-; X64-AVX:       # %bb.0:
-; X64-AVX-NEXT:    vpand {{\.?LCPI[0-9]+_[0-9]+}}(%rip), %xmm0, %xmm0
-; X64-AVX-NEXT:    vpsrad $1, %xmm0, %xmm0
-; X64-AVX-NEXT:    retq
+; X64-AVX1-LABEL: test_128_i64_x_2_18446744065119617024_mask_ashr_1:
+; X64-AVX1:       # %bb.0:
+; X64-AVX1-NEXT:    vpand {{\.?LCPI[0-9]+_[0-9]+}}(%rip), %xmm0, %xmm0
+; X64-AVX1-NEXT:    vpsrad $1, %xmm0, %xmm0
+; X64-AVX1-NEXT:    vpxor %xmm1, %xmm1, %xmm1
+; X64-AVX1-NEXT:    vpblendw {{.*#+}} xmm0 = xmm1[0,1],xmm0[2,3],xmm1[4,5],xmm0[6,7]
+; X64-AVX1-NEXT:    retq
+;
+; X64-AVX2-LABEL: test_128_i64_x_2_18446744065119617024_mask_ashr_1:
+; X64-AVX2:       # %bb.0:
+; X64-AVX2-NEXT:    vpand {{\.?LCPI[0-9]+_[0-9]+}}(%rip), %xmm0, %xmm0
+; X64-AVX2-NEXT:    vpsrad $1, %xmm0, %xmm0
+; X64-AVX2-NEXT:    vpxor %xmm1, %xmm1, %xmm1
+; X64-AVX2-NEXT:    vpblendd {{.*#+}} xmm0 = xmm1[0],xmm0[1],xmm1[2],xmm0[3]
+; X64-AVX2-NEXT:    retq
   %t0 = and <2 x i64> %a0, <i64 18446744065119617024, i64 18446744065119617024>
   %t1 = ashr <2 x i64> %t0, <i64 1, i64 1>
   ret <2 x i64> %t1
 }
 define <2 x i64> @test_128_i64_x_2_18446744065119617024_mask_ashr_32(<2 x i64> %a0) {
 ; X86-SSE2-LABEL: test_128_i64_x_2_18446744065119617024_mask_ashr_32:
 ; X86-SSE2:       # %bb.0:
 ; X86-SSE2-NEXT:    pand {{\.?LCPI[0-9]+_[0-9]+}}, %xmm0