[ADT] Refactor comparison operator in APSInt by compareValues

[ChuanqiXu]

Summary

Now the comparison operator in APSInt would require the signedness of the operands are the same, otherwise the compiler would crash. It is really annoying that I found my code crash at A == B due to the mismatched signedness.

Then I found there is interface called compareValues which handled the mismatched signedness. So I think we should use compareValues to refactor the comparison operator. Both of them do comparison and they should have similar semantics.

[rjmccall]

These assertions are actually protecting against some pretty significant sources of bugs, like trying to evaluate a constant comparison without applying the appropriate conversions on the operands first. APSInt models a consistent integer type of statically-unknown width and signedness; if you're trying to use it as a possibly-negative arbitrary-precision integer, a lot of the behavior is wrong, like the fact that overflow will wrap around instead of extending the storage.

I do think it would be useful to have a type that models arbitrary-precision integer math, if you want to work on that.

[ChuanqiXu]

In D118315#3275007, @rjmccall wrote:

These assertions are actually protecting against some pretty significant sources of bugs, like trying to evaluate a constant comparison without applying the appropriate conversions on the operands first. APSInt models a consistent integer type of statically-unknown width and signedness; if you're trying to use it as a possibly-negative arbitrary-precision integer, a lot of the behavior is wrong, like the fact that overflow will wrap around instead of extending the storage.

I do think it would be useful to have a type that models arbitrary-precision integer math, if you want to work on that.

I am confused about the existence of the method`compareValues`. It handles the bitwidth and signedeness. And the functionality of compareValues looks really like comparison operator to me . I think they should be consistent. If we indeed want the assertion, I think we should have one in the compareValues method.

[rjmccall]

compareValues is a specific request to do a comparison of the represented integer values. It's fine to have API like that which ignores bit-width and signedness; it doesn't change that ordinary comparisons should require no bit-width and signedness differences in order to protect against accidental mis-use.

[ChuanqiXu]

Oh, got it. Thanks for explanation.

llvm/include/llvm/ADT/APSInt.h

APSInt& operator>>=(unsigned Amt) {
     if (IsUnsigned)
       lshrInPlace(Amt);
     else
       ashrInPlace(Amt);
     return *this;
   }
 
   inline bool operator<(const APSInt& RHS) const {
-    assert(IsUnsigned == RHS.IsUnsigned && "Signedness mismatch!");
-    return IsUnsigned ? ult(RHS) : slt(RHS);
+    return compareValues(*this, RHS) < 0;
   }
   inline bool operator>(const APSInt& RHS) const {
-    assert(IsUnsigned == RHS.IsUnsigned && "Signedness mismatch!");
-    return IsUnsigned ? ugt(RHS) : sgt(RHS);
+    return compareValues(*this, RHS) > 0;
   }
   inline bool operator<=(const APSInt& RHS) const {
-    assert(IsUnsigned == RHS.IsUnsigned && "Signedness mismatch!");
-    return IsUnsigned ? ule(RHS) : sle(RHS);
+    return compareValues(*this, RHS) <= 0;
   }
   inline bool operator>=(const APSInt& RHS) const {
-    assert(IsUnsigned == RHS.IsUnsigned && "Signedness mismatch!");
-    return IsUnsigned ? uge(RHS) : sge(RHS);
+    return compareValues(*this, RHS) >= 0;
   }
   inline bool operator==(const APSInt& RHS) const {
-    assert(IsUnsigned == RHS.IsUnsigned && "Signedness mismatch!");
-    return eq(RHS);
+    return compareValues(*this, RHS) == 0;
   }
   inline bool operator!=(const APSInt& RHS) const {
     return !((*this) == RHS);
   }
 
   bool operator==(int64_t RHS) const {
     return compareValues(*this, get(RHS)) == 0;
   }
static inline APSInt getTombstoneKey() {
     return APSInt(DenseMapInfo<APInt, void>::getTombstoneKey());
   }
 
   static unsigned getHashValue(const APSInt &Key) {
     return DenseMapInfo<APInt, void>::getHashValue(Key);
   }
 
   static bool isEqual(const APSInt &LHS, const APSInt &RHS) {
-    return LHS.getBitWidth() == RHS.getBitWidth() &&
-           LHS.isUnsigned() == RHS.isUnsigned() && LHS == RHS;
+    return LHS.getBitWidth() == RHS.getBitWidth() && LHS == RHS;
   }
 };
 
 } // end namespace llvm
 
 #endif

llvm/unittests/ADT/APSIntTest.cpp

TEST(APSIntTest, compareValues) {
   EXPECT_TRUE(APSInt::compareValues(U(7).trunc(32), S(7)) == 0);
   EXPECT_TRUE(APSInt::compareValues(U(8).trunc(32), S(-7)) > 0);
   EXPECT_TRUE(APSInt::compareValues(U(7), S(8).trunc(32)) < 0);
   EXPECT_TRUE(APSInt::compareValues(U(8), S(7).trunc(32)) > 0);
   EXPECT_TRUE(APSInt::compareValues(U(7), S(7).trunc(32)) == 0);
   EXPECT_TRUE(APSInt::compareValues(U(8), S(-7).trunc(32)) > 0);
 }
 
+TEST(APSIntTest, comparisonOperators) {
+  auto U = [](uint64_t V) { return APSInt::getUnsigned(V); };
+  auto S = [](int64_t V) { return APSInt::get(V); };
+
+  // Bit-width matches and is-signed.
+  EXPECT_TRUE(S(7) < S(8));
+  EXPECT_TRUE(S(8) > S(7));
+  EXPECT_TRUE(S(7) == S(7));
+  EXPECT_TRUE(S(-7) < S(8));
+  EXPECT_TRUE(S(8) > S(-7));
+  EXPECT_TRUE(S(-7) == S(-7));
+  EXPECT_TRUE(S(-7) > S(-8));
+  EXPECT_TRUE(S(-8) < S(-7));
+  EXPECT_TRUE(S(-7) == S(-7));
+
+  // Bit-width matches and not is-signed.
+  EXPECT_TRUE(U(7) < U(8));
+  EXPECT_TRUE(U(8) > U(7));
+  EXPECT_TRUE(U(7) == U(7));
+
+  // Bit-width matches and mixed signs.
+  EXPECT_TRUE(U(7) < S(8));
+  EXPECT_TRUE(U(8) > S(7));
+  EXPECT_TRUE(U(7) == S(7));
+  EXPECT_TRUE(U(8) > S(-7));
+
+  // Bit-width mismatch and is-signed.
+  EXPECT_TRUE(S(7).trunc(32) < S(8));
+  EXPECT_TRUE(S(8).trunc(32) > S(7));
+  EXPECT_TRUE(S(7).trunc(32) == S(7));
+  EXPECT_TRUE(S(-7).trunc(32) < S(8));
+  EXPECT_TRUE(S(8).trunc(32) > S(-7));
+  EXPECT_TRUE(S(-7).trunc(32) == S(-7));
+  EXPECT_TRUE(S(-7).trunc(32) > S(-8));
+  EXPECT_TRUE(S(-8).trunc(32) < S(-7));
+  EXPECT_TRUE(S(-7).trunc(32) == S(-7));
+  EXPECT_TRUE(S(7) < S(8).trunc(32));
+  EXPECT_TRUE(S(8) > S(7).trunc(32));
+  EXPECT_TRUE(S(7) == S(7).trunc(32));
+  EXPECT_TRUE(S(-7) < S(8).trunc(32));
+  EXPECT_TRUE(S(8) > S(-7).trunc(32));
+  EXPECT_TRUE(S(-7) == S(-7).trunc(32));
+  EXPECT_TRUE(S(-7) > S(-8).trunc(32));
+  EXPECT_TRUE(S(-8) < S(-7).trunc(32));
+  EXPECT_TRUE(S(-7) == S(-7).trunc(32));
+
+  // Bit-width mismatch and not is-signed.
+  EXPECT_TRUE(U(7) < U(8).trunc(32));
+  EXPECT_TRUE(U(8) > U(7).trunc(32));
+  EXPECT_TRUE(U(7) == U(7).trunc(32));
+  EXPECT_TRUE(U(7).trunc(32) < U(8));
+  EXPECT_TRUE(U(8).trunc(32) > U(7));
+  EXPECT_TRUE(U(7).trunc(32) == U(7));
+
+  // Bit-width mismatch and mixed signs.
+  EXPECT_TRUE(U(7).trunc(32) < S(8));
+  EXPECT_TRUE(U(8).trunc(32) > S(7));
+  EXPECT_TRUE(U(7).trunc(32) == S(7));
+  EXPECT_TRUE(U(8).trunc(32) > S(-7));
+  EXPECT_TRUE(U(7) < S(8).trunc(32));
+  EXPECT_TRUE(U(8) > S(7).trunc(32));
+  EXPECT_TRUE(U(7) == S(7).trunc(32));
+  EXPECT_TRUE(U(8) > S(-7).trunc(32));
+}
+
 TEST(APSIntTest, FromString) {
   EXPECT_EQ(APSInt("1").getExtValue(), 1);
   EXPECT_EQ(APSInt("-1").getExtValue(), -1);
   EXPECT_EQ(APSInt("0").getExtValue(), 0);
   EXPECT_EQ(APSInt("56789").getExtValue(), 56789);
   EXPECT_EQ(APSInt("-1234").getExtValue(), -1234);
 }