[SelectionDAG] Remove an early-out from computeKnownBits for smin/smax

[foad]

Summary

Even if we know nothing about LHS, it can still be useful to know that
smax(LHS, RHS) >= RHS and smin(LHS, RHS) <= RHS.

[foad]

N.B without D87034, this change wouldn't affect any codegen tests. So this is one case where the improved known bits analysis actually makes a difference.

[RKSimon]

llvm/test/CodeGen/X86/avx512-trunc.ll

1020

Is this testing what it means to? I can't remember offhand what the test is for - @craig.topper any ideas?

llvm/test/CodeGen/X86/masked_store_trunc_usat.ll

5199 ↗

(On Diff #289963)

regression?

6420 ↗

(On Diff #289963)

regression?

llvm/test/CodeGen/X86/vector-trunc-usat.ll

4266 ↗

(On Diff #289963)

we appear to have 3 constant loads now instead of 2

[craig.topper]

llvm/test/CodeGen/X86/avx512-trunc.ll

1020

Not sure either. InstCombine simplifies it to a store of all ones.

[foad]

llvm/test/CodeGen/X86/masked_store_trunc_usat.ll

5199 ↗

(On Diff #289963)

Wel, yes... It has spotted that the result of the pminsw is always negative, so rather than XOR with 0x8000 to flip (i.e. clear) the sign bit, it can AND with 0x7fff to clear the sign bit. But unfortunately that means materialising another constant.

I don't know where this XOR -> AND "optimization" happens, or whether it can be finessed.

The other regressions you pointed out below are basically the same issue.

[foad]

llvm/test/CodeGen/X86/masked_store_trunc_usat.ll

5199 ↗

(On Diff #289963)

I don't know where this XOR -> AND "optimization" happens, or whether it can be finessed.

We have done this basically forever, in the XOR case in TargetLowering::SimplifyDemandedBits:

// If one side is a constant, and all of the known set bits on the other
// side are also set in the constant, turn this into an AND, as we know
// the bits will be cleared.
//    e.g. (X | C1) ^ C2 --> (X | C1) & ~C2 iff (C1&C2) == C2

It seems to me that this is just bad luck, that we transform X^0x8000 into X&0x7FFF, but that happens to regress code quality because now we can't share with another use of the constant 0x8000. Is there any systematic way of fixing this, e.g. by doing the reverse transformation once we know what constants are already available in registers? Or can I commit this patch even with a known regression like this? After all, I'm sure if I looked hard enough I could find another test that got better by luck instead of worse.

[foad]

llvm/test/CodeGen/X86/masked_store_trunc_usat.ll

5199 ↗

(On Diff #289963)

If you buy the argument that SimplifyDemandedBits replacing XOR -> AND is not an optimization and is unhelpful in this case, D87464 + D87465 is my attempt at fixing that. If I rebase this patch on those two then all the bad diffs go away, leaving only the good diffs in avx512-trunc.ll.

[RKSimon]

llvm/test/CodeGen/X86/avx512-trunc.ll

1020

The test was added as part of D45315 - @ArturGainullin hasn't been an active for some time afaict.

I think we might be able to get away with adjusting the -1 splat to be a non-uniform mix of -ve constant values.

[RKSimon]

reverse ping?

[RKSimon]

llvm/test/CodeGen/X86/avx512-trunc.ll

1020

Any recommendations on what to do with these tests?

[foad]

Rebase. D87236 seems to have fixed the code quality regressions.

[RKSimon]

@foad Please can you rebase? I think I've replaced the dodgy test with something useful now

[foad]

Rebase.

[RKSimon]

LGTM - cheers!

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

if (CstLow && CstHigh) {
         if (ValueLow.isNonNegative() && ValueHigh.isNonNegative()) {
           Known.Zero.setHighBits(MinSignBits);
           break;
         }
       }
     }
 
     Known = computeKnownBits(Op.getOperand(0), DemandedElts, Depth + 1);
-    if (Known.isUnknown()) break; // Early-out
     Known2 = computeKnownBits(Op.getOperand(1), DemandedElts, Depth + 1);
     if (IsMax)
       Known = KnownBits::smax(Known, Known2);
     else
       Known = KnownBits::smin(Known, Known2);
     break;
   }
   case ISD::FrameIndex:

llvm/test/CodeGen/X86/avx512-trunc.ll

; ALL-NEXT:    retq
   %x5 = select <16 x i1> %x3, <16 x i32> %x2, <16 x i32> <i32 65535, i32 65535, i32 65535, i32 65535, i32 65535, i32 65535, i32 65535, i32 65535, i32 65535, i32 65535, i32 65535, i32 65535, i32 65535, i32 65535, i32 65535, i32 65535>
   %x6 = trunc <16 x i32> %x5 to <16 x i16>
   ret <16 x i16> %x6
 }
 
 define void @negative_test1_smax_usat_trunc_wb_256_mem(<16 x i16> %i, <16 x i8>* %res) {
 ; KNL-LABEL: negative_test1_smax_usat_trunc_wb_256_mem:
 ; KNL:       ## %bb.0:
-; KNL-NEXT:    vpxor %xmm1, %xmm1, %xmm1
-; KNL-NEXT:    vpmaxsw %ymm1, %ymm0, %ymm0
-; KNL-NEXT:    vpcmpeqd %ymm1, %ymm1, %ymm1
-; KNL-NEXT:    vpminsw %ymm1, %ymm0, %ymm0
-; KNL-NEXT:    vpmovzxwd {{.*#+}} zmm0 = ymm0[0],zero,ymm0[1],zero,ymm0[2],zero,ymm0[3],zero,ymm0[4],zero,ymm0[5],zero,ymm0[6],zero,ymm0[7],zero,ymm0[8],zero,ymm0[9],zero,ymm0[10],zero,ymm0[11],zero,ymm0[12],zero,ymm0[13],zero,ymm0[14],zero,ymm0[15],zero
+; KNL-NEXT:    vpbroadcastd {{.*#+}} zmm0 = [255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255]
 ; KNL-NEXT:    vpmovdb %zmm0, (%rdi)
 ; KNL-NEXT:    vzeroupper
 ; KNL-NEXT:    retq
 ;
 ; SKX-LABEL: negative_test1_smax_usat_trunc_wb_256_mem:
 ; SKX:       ## %bb.0:
-; SKX-NEXT:    vpxor %xmm1, %xmm1, %xmm1
-; SKX-NEXT:    vpmaxsw %ymm1, %ymm0, %ymm0
-; SKX-NEXT:    vpcmpeqd %ymm1, %ymm1, %ymm1
-; SKX-NEXT:    vpminsw %ymm1, %ymm0, %ymm0
+; SKX-NEXT:    vpcmpeqd %ymm0, %ymm0, %ymm0
 ; SKX-NEXT:    vpmovwb %ymm0, (%rdi)
 ; SKX-NEXT:    vzeroupper
 ; SKX-NEXT:    retq
   %x1 = icmp sgt <16 x i16> %i, <i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0>
   %x2 = select <16 x i1> %x1, <16 x i16> %i, <16 x i16> <i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0, i16 0>
   %x3 = icmp slt <16 x i16> %x2, <i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1>
   %x5 = select <16 x i1> %x3, <16 x i16> %x2, <16 x i16> <i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1>
   %x6 = trunc <16 x i16> %x5 to <16 x i8>
   store <16 x i8> %x6, <16 x i8>* %res, align 1
   ret void
 }

llvm/test/CodeGen/X86/known-bits-vector.ll

 }
 
 define <4 x float> @knownbits_smax_smin_shuffle_uitofp(<4 x i32> %a0) {
 ; X32-LABEL: knownbits_smax_smin_shuffle_uitofp:
 ; X32:       # %bb.0:
 ; X32-NEXT:    vpminsd {{\.LCPI.*}}, %xmm0, %xmm0
 ; X32-NEXT:    vpmaxsd {{\.LCPI.*}}, %xmm0, %xmm0
 ; X32-NEXT:    vpshufd {{.*#+}} xmm0 = xmm0[0,0,3,3]
-; X32-NEXT:    vpblendw {{.*#+}} xmm1 = xmm0[0],mem[1],xmm0[2],mem[3],xmm0[4],mem[5],xmm0[6],mem[7]
-; X32-NEXT:    vpsrld $16, %xmm0, %xmm0
-; X32-NEXT:    vpblendw {{.*#+}} xmm0 = xmm0[0],mem[1],xmm0[2],mem[3],xmm0[4],mem[5],xmm0[6],mem[7]
-; X32-NEXT:    vsubps {{\.LCPI.*}}, %xmm0, %xmm0
-; X32-NEXT:    vaddps %xmm0, %xmm1, %xmm0
+; X32-NEXT:    vcvtdq2ps %xmm0, %xmm0
 ; X32-NEXT:    retl
 ;
 ; X64-LABEL: knownbits_smax_smin_shuffle_uitofp:
 ; X64:       # %bb.0:
 ; X64-NEXT:    vpminsd {{.*}}(%rip), %xmm0, %xmm0
 ; X64-NEXT:    vpmaxsd {{.*}}(%rip), %xmm0, %xmm0
 ; X64-NEXT:    vpshufd {{.*#+}} xmm0 = xmm0[0,0,3,3]
-; X64-NEXT:    vpblendw {{.*#+}} xmm1 = xmm0[0],mem[1],xmm0[2],mem[3],xmm0[4],mem[5],xmm0[6],mem[7]
-; X64-NEXT:    vpsrld $16, %xmm0, %xmm0
-; X64-NEXT:    vpblendw {{.*#+}} xmm0 = xmm0[0],mem[1],xmm0[2],mem[3],xmm0[4],mem[5],xmm0[6],mem[7]
-; X64-NEXT:    vsubps {{.*}}(%rip), %xmm0, %xmm0
-; X64-NEXT:    vaddps %xmm0, %xmm1, %xmm0
+; X64-NEXT:    vcvtdq2ps %xmm0, %xmm0
 ; X64-NEXT:    retq
   %1 = call <4 x i32> @llvm.x86.sse41.pminsd(<4 x i32> %a0, <4 x i32> <i32 0, i32 -65535, i32 -65535, i32 0>)
   %2 = call <4 x i32> @llvm.x86.sse41.pmaxsd(<4 x i32> %1, <4 x i32> <i32 65535, i32 -1, i32 -1, i32 131071>)
   %3 = shufflevector <4 x i32> %2, <4 x i32> undef, <4 x i32> <i32 0, i32 0, i32 3, i32 3>
   %4 = uitofp <4 x i32> %3 to <4 x float>
   ret <4 x float> %4
 }
 declare <4 x i32> @llvm.x86.sse41.pmaxsd(<4 x i32>, <4 x i32>) nounwind readnone