[DAGCombine] optimizeSetCCOfSignedTruncationCheck(): handle ule,ugt CondCodes.

[lebedev.ri]

Summary

A follow-up for D49266 / rL337166.

At least one of these cases is more canonical,
so we really do have to handle it.
https://godbolt.org/g/pkzP3X
https://rise4fun.com/Alive/pQyhZZ

We won't get to these cases with I1 being -1,
as that will be constant-folded to true or false.

I'm also not sure we actually hit the 'ule' case,
but i think the worst think that could happen is that being dead code.

[lebedev.ri]

While there, a question: do we want to keep producing cmp with immediate in the unsigned case, or should we be producing movzx?
https://godbolt.org/g/yY2Gnp

[lebedev.ri]

I will probably turn this into post-commit review.

This is a pretty uncontroversial follow-up for my own previous patch, the alive agrees that this is correct,
and the only edge-case which you'd think we can have (-1 value) we won't get as that is constant-folded, and there is a test that checks that.

[spatel]

LGTM.

lib/CodeGen/SelectionDAG/TargetLowering.cpp

1888–1889

Nit: I don't think LLVM has any hard guidelines for this, but I've always felt that if any clause in an if-else sequence has braces, then all clauses should have braces for consistency. I don't see any value in omitting them in those situations, just more potential confusion/risk.

[lebedev.ri]

In D49497#1176667, @spatel wrote:

LGTM.

Thank you for the review!

lib/CodeGen/SelectionDAG/TargetLowering.cpp

1888–1889

Since in LLVM .clang-format case the {} will be on the same line (as in, not on the separate/new lines),
i think that would be fine.

lib/CodeGen/SelectionDAG/TargetLowering.cpp

 /// KeptBits won't be bitwidth(x), that will be constant-folded to true/false.
 /// KeptBits also can't be 1, that would have been folded to  %x dstcond 0
 /// We will unfold it into the natural trunc+sext pattern:
 ///   ((%x << C) a>> C) dstcond %x
 /// Where  C = bitwidth(x) - KeptBits  and  C u< bitwidth(x)
 SDValue TargetLowering::optimizeSetCCOfSignedTruncationCheck(
     EVT SCCVT, SDValue N0, SDValue N1, ISD::CondCode Cond, DAGCombinerInfo &DCI,
     const SDLoc &DL) const {
-  ISD::CondCode NewCond;
-  if (Cond == ISD::CondCode::SETULT)
-    NewCond = ISD::CondCode::SETEQ;
-  else if (Cond == ISD::CondCode::SETUGE)
-    NewCond = ISD::CondCode::SETNE;
-  else
-    return SDValue();
-
   // We must be comparing with a constant.
   ConstantSDNode *C1;
   if (!(C1 = dyn_cast<ConstantSDNode>(N1)))
     return SDValue();
 
   // N0 should be:  add %x, (1 << (KeptBits-1))
   if (N0->getOpcode() != ISD::ADD)
     return SDValue();
 
   // And we must be 'add'ing a constant.
   ConstantSDNode *C01;
   if (!(C01 = dyn_cast<ConstantSDNode>(N0->getOperand(1))))
     return SDValue();
 
   SDValue X = N0->getOperand(0);
   EVT XVT = X.getValueType();
 
   // Validate constants ...
 
-  const APInt &I1 = C1->getAPIntValue();
+  APInt I1 = C1->getAPIntValue();
+
+  ISD::CondCode NewCond;
+  if (Cond == ISD::CondCode::SETULT)
+    NewCond = ISD::CondCode::SETEQ;
+  else if (Cond == ISD::CondCode::SETULE) {
+    NewCond = ISD::CondCode::SETEQ;
+    // But need to 'canonicalize' the constant.
+    I1 += 1;
+  } else if (Cond == ISD::CondCode::SETUGT) {
+    NewCond = ISD::CondCode::SETNE;
+    // But need to 'canonicalize' the constant.
+    I1 += 1;
+  } else if (Cond == ISD::CondCode::SETUGE)
+    NewCond = ISD::CondCode::SETNE;
+  else
+    return SDValue();
+
   const APInt &I01 = C01->getAPIntValue();
   // Both of them must be power-of-two, and the constant from setcc is bigger.
   if (!(I1.ugt(I01) && I1.isPowerOf2() && I01.isPowerOf2()))
     return SDValue();
 
   // They are power-of-two, so which bit is set?
   const unsigned KeptBits = I1.logBase2();
   const unsigned KeptBitsMinusOne = I01.logBase2();

test/CodeGen/AArch64/lack-of-signed-truncation-check.ll

; CHECK-NEXT:    ret
   %tmp1 = icmp uge i64 %tmp0, 256 ; 1U << 8
   ret i1 %tmp1
 }
 
 ; Slightly more canonical variant
 define i1 @add_ugtcmp_i16_i8(i16 %x) nounwind {
 ; CHECK-LABEL: add_ugtcmp_i16_i8:
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    add w8, w0, #128 // =128
+; CHECK-NEXT:    sxtb w8, w0
 ; CHECK-NEXT:    and w8, w8, #0xffff
-; CHECK-NEXT:    cmp w8, #255 // =255
-; CHECK-NEXT:    cset w0, hi
+; CHECK-NEXT:    cmp w8, w0, uxth
+; CHECK-NEXT:    cset w0, ne
 ; CHECK-NEXT:    ret
   %tmp0 = add i16 %x, 128 ; 1U << (8-1)
   %tmp1 = icmp ugt i16 %tmp0, 255 ; (1U << 8) - 1
   ret i1 %tmp1
 }
 
 ; Negative tests
 ; ---------------------------------------------------------------------------- ;

test/CodeGen/X86/lack-of-signed-truncation-check.ll

; X64-NEXT:    retq
   %tmp1 = icmp uge i64 %tmp0, 256 ; 1U << 8
   ret i1 %tmp1
 }
 
 ; Slightly more canonical variant
 define i1 @add_ugtcmp_i16_i8(i16 %x) nounwind {
 ; X86-LABEL: add_ugtcmp_i16_i8:
 ; X86:       # %bb.0:
-; X86-NEXT:    movl $128, %eax
-; X86-NEXT:    addl {{[0-9]+}}(%esp), %eax
-; X86-NEXT:    movzwl %ax, %eax
-; X86-NEXT:    cmpl $255, %eax
-; X86-NEXT:    seta %al
+; X86-NEXT:    movzwl {{[0-9]+}}(%esp), %eax
+; X86-NEXT:    movsbl %al, %ecx
+; X86-NEXT:    cmpw %ax, %cx
+; X86-NEXT:    setne %al
 ; X86-NEXT:    retl
 ;
 ; X64-LABEL: add_ugtcmp_i16_i8:
 ; X64:       # %bb.0:
-; X64-NEXT:    subl $-128, %edi
-; X64-NEXT:    movzwl %di, %eax
-; X64-NEXT:    cmpl $255, %eax
-; X64-NEXT:    seta %al
+; X64-NEXT:    movsbl %dil, %eax
+; X64-NEXT:    cmpw %di, %ax
+; X64-NEXT:    setne %al
 ; X64-NEXT:    retq
   %tmp0 = add i16 %x, 128 ; 1U << (8-1)
   %tmp1 = icmp ugt i16 %tmp0, 255 ; (1U << 8) - 1
   ret i1 %tmp1
 }
 
 ; Negative tests
 ; ---------------------------------------------------------------------------- ;