Fix icmp lowering

[chfast]

Summary

During icmp lowering it can happen that a constant value can be larger than expected (see the code around the change).
APInt::getMinSignedBits() must be checked again as the shift before can change the constant sign to positive.
I'm not sure it is the best fix possible though.

[chfast]

Ping.

[chfast]

Ping.

[chfast]

Ping.

[chfast]

Ping.

[hfinkel]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

1727 ↗

(On Diff #24120)

Why 64? Shouldn't this depend on the size of the type?

test/CodeGen/Generic/icmp-illegal.ll

4 ↗

(On Diff #24120)

What is this test testing? Did these crash previously?

[chfast]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

1727 ↗

(On Diff #24120)

This check is needed to later NewC.getSExtValue() be always correct. isLegalICmpImmediate checks the real value.

test/CodeGen/Generic/icmp-illegal.ll

4 ↗

(On Diff #24120)

Previously that caused an assert failure in NewC.getSExtValue().

[hfinkel]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

1727 ↗

(On Diff #24120)

Sure, but why 64?

test/CodeGen/Generic/icmp-illegal.ll

4 ↗

(On Diff #24120)

Okay, thanks.

[chfast]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

1727 ↗

(On Diff #24120)

If it less than 64 you can fit the value in int64_t. If not you cannot. It's all about C++ types, not LLVM types.

[hfinkel]

LGTM.

lib/CodeGen/SelectionDAG/TargetLowering.cpp

1727 ↗

(On Diff #24120)

Okay, thanks. It is that NewC.getSExtValue() gives the int64_t.

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

if (C1.getMinSignedBits() <= 64 &&
         if (AdjOne) {
           ShiftBits = C1.countTrailingOnes();
           NewC = NewC + 1;
           NewCond = (Cond == ISD::SETULE) ? ISD::SETULT : ISD::SETUGE;
         } else {
           ShiftBits = C1.countTrailingZeros();
         }
         NewC = NewC.lshr(ShiftBits);
-        if (ShiftBits && isLegalICmpImmediate(NewC.getSExtValue())) {
+        if (ShiftBits && NewC.getMinSignedBits() <= 64 &&
+          isLegalICmpImmediate(NewC.getSExtValue())) {
           EVT ShiftTy = DCI.isBeforeLegalize() ?
             getPointerTy() : getShiftAmountTy(N0.getValueType());
           EVT CmpTy = N0.getValueType();
           SDValue Shift = DAG.getNode(ISD::SRL, dl, CmpTy, N0,
                                       DAG.getConstant(ShiftBits, dl, ShiftTy));
           SDValue CmpRHS = DAG.getConstant(NewC, dl, CmpTy);
           return DAG.getSetCC(dl, VT, Shift, CmpRHS, NewCond);
         }

llvm/trunk/test/CodeGen/Generic/icmp-illegal.ll

+
+; RUN: llc < %s | FileCheck %s
+
+; CHECK-LABEL: test_ult
+define i1 @test_ult(i256 %a) nounwind {
+  %1 = icmp ult i256 %a, -6432394258550908438
+  ret i1 %1
+}
+
+; CHECK-LABEL: test_ule
+define i1 @test_ule(i256 %a) nounwind {
+  %1 = icmp ule i256 %a, -6432394258550908438
+  ret i1 %1
+}
+
+; CHECK-LABEL: test_ugt
+define i1 @test_ugt(i256 %a) nounwind {
+  %1 = icmp ugt i256 %a, -6432394258550908438
+  ret i1 %1
+}
+
+; CHECK-LABEL: test_uge
+define i1 @test_uge(i256 %a) nounwind {
+  %1 = icmp uge i256 %a, -6432394258550908438
+  ret i1 %1
+}
+
+; CHECK-LABEL: test_slt
+define i1 @test_slt(i256 %a) nounwind {
+  %1 = icmp slt i256 %a, -6432394258550908438
+  ret i1 %1
+}
+
+; CHECK-LABEL: test_sle
+define i1 @test_sle(i256 %a) nounwind {
+  %1 = icmp sle i256 %a, -6432394258550908438
+  ret i1 %1
+}
+
+; CHECK-LABEL: test_sgt
+define i1 @test_sgt(i256 %a) nounwind {
+  %1 = icmp sgt i256 %a, -6432394258550908438
+  ret i1 %1
+}
+
+; CHECK-LABEL: test_sge
+define i1 @test_sge(i256 %a) nounwind {
+  %1 = icmp sge i256 %a, -6432394258550908438
+  ret i1 %1
+}