[TargetLowering] Fix undef vector element issue with true/false result handling

[RKSimon]

Summary

Fixed an issue with vector usage of TargetLowering::isConstTrueVal / TargetLowering::isConstFalseVal boolean result matching.

The comment says that we shouldn't handle constant splat vectors with undef elements. But the the actual code was returning false if the build vector contained no undef elements....

This current iteration of the patch flips the test so that it matches the comment and only accepts build vectors with no undefs. But since this has been like this since 2014, should we take it as a sign that undefs elements are not an issue and I should update the patch to not care about undefs at all?

The change has also unearthed a couple of missed opportunities in AVX512 comparison code that will need to be addressed.

[hfinkel]

My inclination is to leave the code as is, and update the comment to match. If we have all undefs, then we return false (presumably, we have better ways to fold an all-undef operand), but a few undefs are fine.

On the other hand, do we generally produce better code if we don't handle undefs at all?

[RKSimon]

In D26031#581237, @hfinkel wrote:

My inclination is to leave the code as is, and update the comment to match. If we have all undefs, then we return false (presumably, we have better ways to fold an all-undef operand), but a few undefs are fine.

OK - I'll update the patch to completely ignore the UndefElements arg - its not necessary (getConstantSplatNode returns NULL if its a build vector of all undefs).

On the other hand, do we generally produce better code if we don't handle undefs at all?

You can see examples of the codegen delta in the patch (the v8i1-masks.ll cases are similar to the real world code that led me to this patch in the first place). The AVX512 vpcmpgtd() -> knotw(vpcmpled()) is a regression, but minimal compared to the gains elsewhere and should be easy to fix later on.

[RKSimon]

Updated to not care at about some (but not all!) undefs in a splatted build vector constant.

[delena]

I agree that undef elements may be ignored in constant splat.

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

if (!N)
     return false;
 
   const ConstantSDNode *CN = dyn_cast<ConstantSDNode>(N);
   if (!CN) {
     const BuildVectorSDNode *BV = dyn_cast<BuildVectorSDNode>(N);
     if (!BV)
       return false;
 
-    BitVector UndefElements;
-    CN = BV->getConstantSplatNode(&UndefElements);
-    // Only interested in constant splats, and we don't try to handle undef
-    // elements in identifying boolean constants.
-    if (!CN || UndefElements.none())
+    // Only interested in constant splats, we don't care about undef
+    // elements in identifying boolean constants and getConstantSplatNode
+    // returns NULL if all ops are undef;
+    CN = BV->getConstantSplatNode();
+    if (!CN)
       return false;
   }
 
   switch (getBooleanContents(N->getValueType(0))) {
   case UndefinedBooleanContent:
     return CN->getAPIntValue()[0];
   case ZeroOrOneBooleanContent:
     return CN->isOne();
if (!N)
     return false;
 
   const ConstantSDNode *CN = dyn_cast<ConstantSDNode>(N);
   if (!CN) {
     const BuildVectorSDNode *BV = dyn_cast<BuildVectorSDNode>(N);
     if (!BV)
       return false;
 
-    BitVector UndefElements;
-    CN = BV->getConstantSplatNode(&UndefElements);
-    // Only interested in constant splats, and we don't try to handle undef
-    // elements in identifying boolean constants.
-    if (!CN || UndefElements.none())
+    // Only interested in constant splats, we don't care about undef
+    // elements in identifying boolean constants and getConstantSplatNode
+    // returns NULL if all ops are undef;
+    CN = BV->getConstantSplatNode();
+    if (!CN)
       return false;
   }
 
   if (getBooleanContents(N->getValueType(0)) == UndefinedBooleanContent)
     return !CN->getAPIntValue()[0];
 
   return CN->isNullValue();
 }

llvm/trunk/test/CodeGen/X86/avx512-ext.ll

; SKX-NEXT:    retq
   %mask_b = trunc <8 x i16>%a to <8 x i1>
   %mask = bitcast <8 x i1> %mask_b to i8
   ret i8 %mask
 }
 
 define <8 x i32> @sext_8i1_8i32(<8 x i32> %a1, <8 x i32> %a2) nounwind {
 ; KNL-LABEL: sext_8i1_8i32:
 ; KNL:       ## BB#0:
-; KNL-NEXT:    ## kill: %YMM1<def> %YMM1<kill> %ZMM1<def>
-; KNL-NEXT:    ## kill: %YMM0<def> %YMM0<kill> %ZMM0<def>
-; KNL-NEXT:    vpcmpgtd %zmm0, %zmm1, %k0
-; KNL-NEXT:    knotw %k0, %k1
-; KNL-NEXT:    vpternlogd $255, %zmm0, %zmm0, %zmm0
-; KNL-NEXT:    vmovdqa64 %zmm0, %zmm0 {%k1} {z}
-; KNL-NEXT:    vpmovqd %zmm0, %ymm0
+; KNL-NEXT:    vpcmpgtd %ymm0, %ymm1, %ymm0
+; KNL-NEXT:    vpcmpeqd %ymm1, %ymm1, %ymm1
+; KNL-NEXT:    vpxor %ymm1, %ymm0, %ymm0
 ; KNL-NEXT:    retq
 ;
 ; SKX-LABEL: sext_8i1_8i32:
 ; SKX:       ## BB#0:
-; SKX-NEXT:    vpcmpgtd %ymm0, %ymm1, %k0
-; SKX-NEXT:    knotb %k0, %k0
+; SKX-NEXT:    vpcmpled %ymm0, %ymm1, %k0
 ; SKX-NEXT:    vpmovm2d %k0, %ymm0
 ; SKX-NEXT:    retq
   %x = icmp slt <8 x i32> %a1, %a2
   %x1 = xor <8 x i1>%x, <i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true>
   %y = sext <8 x i1> %x1 to <8 x i32>
   ret <8 x i32> %y
 }
 

llvm/trunk/test/CodeGen/X86/avx512-vec-cmp.ll

; CHECK-NEXT:    retq
   %conv = zext <16 x i1> %cmpvector_i to <16 x i32>
   ret <16 x i32> %conv
 }
 
 define <16 x i32> @test14(<16 x i32>%a, <16 x i32>%b) {
 ; CHECK-LABEL: test14:
 ; CHECK:       ## BB#0:
 ; CHECK-NEXT:    vpsubd %zmm1, %zmm0, %zmm1
-; CHECK-NEXT:    vpcmpgtd %zmm0, %zmm1, %k1
+; CHECK-NEXT:    vpcmpled %zmm0, %zmm1, %k0
+; CHECK-NEXT:    knotw %k0, %k1
 ; CHECK-NEXT:    vmovdqa32 %zmm1, %zmm0 {%k1} {z}
 ; CHECK-NEXT:    retq
   %sub_r = sub <16 x i32> %a, %b
   %cmp.i2.i = icmp sgt <16 x i32> %sub_r, %a
   %sext.i3.i = sext <16 x i1> %cmp.i2.i to <16 x i32>
   %mask = icmp eq <16 x i32> %sext.i3.i, zeroinitializer
   %res = select <16 x i1> %mask, <16 x i32> zeroinitializer, <16 x i32> %sub_r
   ret <16 x i32>%res
 }
 
 define <8 x i64> @test15(<8 x i64>%a, <8 x i64>%b) {
 ; CHECK-LABEL: test15:
 ; CHECK:       ## BB#0:
 ; CHECK-NEXT:    vpsubq %zmm1, %zmm0, %zmm1
-; CHECK-NEXT:    vpcmpgtq %zmm0, %zmm1, %k1
+; CHECK-NEXT:    vpcmpleq %zmm0, %zmm1, %k0
+; CHECK-NEXT:    knotw %k0, %k1
 ; CHECK-NEXT:    vmovdqa64 %zmm1, %zmm0 {%k1} {z}
 ; CHECK-NEXT:    retq
   %sub_r = sub <8 x i64> %a, %b
   %cmp.i2.i = icmp sgt <8 x i64> %sub_r, %a
   %sext.i3.i = sext <8 x i1> %cmp.i2.i to <8 x i64>
   %mask = icmp eq <8 x i64> %sext.i3.i, zeroinitializer
   %res = select <8 x i1> %mask, <8 x i64> zeroinitializer, <8 x i64> %sub_r
   ret <8 x i64>%res

llvm/trunk/test/CodeGen/X86/v8i1-masks.ll

 }
 
 define void @neg_masks(<8 x float>* %a, <8 x float>* %b, <8 x float>* %c) nounwind uwtable noinline ssp {
 ; X32-LABEL: neg_masks:
 ; X32:       ## BB#0:
 ; X32-NEXT:    movl {{[0-9]+}}(%esp), %eax
 ; X32-NEXT:    movl {{[0-9]+}}(%esp), %ecx
 ; X32-NEXT:    vmovups (%ecx), %ymm0
-; X32-NEXT:    vcmpltps (%eax), %ymm0, %ymm0
-; X32-NEXT:    vmovaps {{.*#+}} ymm1 = [1,1,1,1,1,1,1,1]
-; X32-NEXT:    vxorps %ymm1, %ymm0, %ymm0
-; X32-NEXT:    vandps %ymm1, %ymm0, %ymm0
+; X32-NEXT:    vcmpnltps (%eax), %ymm0, %ymm0
+; X32-NEXT:    vandps LCPI1_0, %ymm0, %ymm0
 ; X32-NEXT:    vmovaps %ymm0, (%eax)
 ; X32-NEXT:    vzeroupper
 ; X32-NEXT:    retl
 ;
 ; X64-LABEL: neg_masks:
 ; X64:       ## BB#0:
 ; X64-NEXT:    vmovups (%rsi), %ymm0
-; X64-NEXT:    vcmpltps (%rdi), %ymm0, %ymm0
-; X64-NEXT:    vmovaps {{.*#+}} ymm1 = [1,1,1,1,1,1,1,1]
-; X64-NEXT:    vxorps %ymm1, %ymm0, %ymm0
-; X64-NEXT:    vandps %ymm1, %ymm0, %ymm0
+; X64-NEXT:    vcmpnltps (%rdi), %ymm0, %ymm0
+; X64-NEXT:    vandps {{.*}}(%rip), %ymm0, %ymm0
 ; X64-NEXT:    vmovaps %ymm0, (%rax)
 ; X64-NEXT:    vzeroupper
 ; X64-NEXT:    retq
   %v0 = load <8 x float>, <8 x float>* %a, align 16
   %v1 = load <8 x float>, <8 x float>* %b, align 16
   %m0 = fcmp olt <8 x float> %v1, %v0
   %mand = xor <8 x i1> %m0, <i1 1, i1 1, i1 1, i1 1, i1 1, i1 1, i1 1, i1 1>
   %r = zext <8 x i1> %mand to <8 x i32>
   store <8 x i32> %r, <8 x i32>* undef, align 32
   ret void
 }