Automerge: [APFloat] Properly implement DoubleAPFloat::compareAbsoluteValue

The prior implementation would treat X+Y and X-Y as having equal
magnitude.  Rework the implementation to be more resilient.

Author: majnemer

llvm/include/llvm/ADT/APFloat.h

@@ -1045,6 +1045,8 @@ class APFloat : public APFloatBase {
   explicit APFloat(DoubleAPFloat F, const fltSemantics &S)
       : U(std::move(F), S) {}
 
+  // Compares the absolute value of this APFloat with another.  Both operands
+  // must be finite non-zero.
   cmpResult compareAbsoluteValue(const APFloat &RHS) const {
     assert(&getSemantics() == &RHS.getSemantics() &&
            "Should only compare APFloats with the same semantics");
@@ -1408,6 +1410,8 @@ class APFloat : public APFloatBase {
     return Res == cmpGreaterThan || Res == cmpEqual;
   }
 
+  // IEEE comparison with another floating point number (NaNs compare unordered,
+  // 0==-0).
   cmpResult compare(const APFloat &RHS) const {
     assert(&getSemantics() == &RHS.getSemantics() &&
            "Should only compare APFloats with the same semantics");

llvm/lib/Support/APFloat.cpp

@@ -5285,23 +5285,56 @@ void DoubleAPFloat::changeSign() {
 
 APFloat::cmpResult
 DoubleAPFloat::compareAbsoluteValue(const DoubleAPFloat &RHS) const {
-  auto Result = Floats[0].compareAbsoluteValue(RHS.Floats[0]);
-  if (Result != cmpEqual)
-    return Result;
-  Result = Floats[1].compareAbsoluteValue(RHS.Floats[1]);
-  if (Result == cmpLessThan || Result == cmpGreaterThan) {
-    auto Against = Floats[0].isNegative() ^ Floats[1].isNegative();
-    auto RHSAgainst = RHS.Floats[0].isNegative() ^ RHS.Floats[1].isNegative();
-    if (Against && !RHSAgainst)
-      return cmpLessThan;
-    if (!Against && RHSAgainst)
+  // Compare absolute values of the high parts.
+  const cmpResult HiPartCmp = Floats[0].compareAbsoluteValue(RHS.Floats[0]);
+  if (HiPartCmp != cmpEqual)
+    return HiPartCmp;
+
+  // Zero, regardless of sign, is equal.
+  if (Floats[1].isZero() && RHS.Floats[1].isZero())
+    return cmpEqual;
+
+  // At this point, |this->Hi| == |RHS.Hi|.
+  // The magnitude is |Hi+Lo| which is Hi+|Lo| if signs of Hi and Lo are the
+  // same, and Hi-|Lo| if signs are different.
+  const bool ThisIsSubtractive =
+      Floats[0].isNegative() != Floats[1].isNegative();
+  const bool RHSIsSubtractive =
+      RHS.Floats[0].isNegative() != RHS.Floats[1].isNegative();
+
+  // Case 1: The low part of 'this' is zero.
+  if (Floats[1].isZero())
+    // We are comparing |Hi| vs. |Hi| ± |RHS.Lo|.
+    // If RHS is subtractive, its magnitude is smaller.
+    // If RHS is additive, its magnitude is larger.
+    return RHSIsSubtractive ? cmpGreaterThan : cmpLessThan;
+
+  // Case 2: The low part of 'RHS' is zero (and we know 'this' is not).
+  if (RHS.Floats[1].isZero())
+    // We are comparing |Hi| ± |This.Lo| vs. |Hi|.
+    // If 'this' is subtractive, its magnitude is smaller.
+    // If 'this' is additive, its magnitude is larger.
+    return ThisIsSubtractive ? cmpLessThan : cmpGreaterThan;
+
+  // If their natures differ, the additive one is larger.
+  if (ThisIsSubtractive != RHSIsSubtractive)
+    return ThisIsSubtractive ? cmpLessThan : cmpGreaterThan;
+
+  // Case 3: Both are additive (Hi+|Lo|) or both are subtractive (Hi-|Lo|).
+  // The comparison now depends on the magnitude of the low parts.
+  const cmpResult LoPartCmp = Floats[1].compareAbsoluteValue(RHS.Floats[1]);
+
+  if (ThisIsSubtractive) {
+    // Both are subtractive (Hi-|Lo|), so the comparison of |Lo| is inverted.
+    if (LoPartCmp == cmpLessThan)
       return cmpGreaterThan;
-    if (!Against && !RHSAgainst)
-      return Result;
-    if (Against && RHSAgainst)
-      return (cmpResult)(cmpLessThan + cmpGreaterThan - Result);
+    if (LoPartCmp == cmpGreaterThan)
+      return cmpLessThan;
   }
-  return Result;
+
+  // If additive, the comparison of |Lo| is direct.
+  // If equal, they are equal.
+  return LoPartCmp;
 }
 
 APFloat::fltCategory DoubleAPFloat::getCategory() const {

llvm/unittests/ADT/APFloatTest.cpp

@@ -6207,6 +6207,102 @@ TEST(APFloatTest, PPCDoubleDoubleCompare) {
   }
 }
 
+namespace PPCDoubleDoubleCompareAbsoluteValueTestDetails {
+struct TestCase {
+  DD LHS;
+  DD RHS;
+  APFloat::cmpResult Result;
+};
+
+auto testCases() {
+  static constexpr auto CompareAbsoluteValueTestCases = std::array{
+      TestCase{
+          {1.0, 0.0},
+          {1.0, 0.0},
+          APFloat::cmpEqual,
+      },
+      TestCase{
+          {1.0, -0.0},
+          {1.0, +0.0},
+          APFloat::cmpEqual,
+      },
+      TestCase{
+          {1.0, 0.0},
+          {0x1.0000000000001p+0, 0.0},
+          APFloat::cmpLessThan,
+      },
+      TestCase{
+          {0x1.0000000000001p+0, 0.0},
+          {1.0, 0.0},
+          APFloat::cmpGreaterThan,
+      },
+      TestCase{
+          {0x1.0000000000001p+0, +0x1p-1074},
+          {1.0, -0x1p-1074},
+          APFloat::cmpGreaterThan,
+      },
+      TestCase{
+          {0x1.0000000000001p+0, -0x1p-1074},
+          {1.0, +0x1p-1074},
+          APFloat::cmpGreaterThan,
+      },
+      TestCase{
+          {1.0, 0.0},
+          {1.0, -0x1p-1074},
+          APFloat::cmpGreaterThan,
+      },
+      TestCase{
+          {1.0, 0.0},
+          {1.0, +0x1p-1074},
+          APFloat::cmpLessThan,
+      },
+      TestCase{
+          {1.0, +0x1p-1073},
+          {1.0, -0x1p-1074},
+          APFloat::cmpGreaterThan,
+      },
+      TestCase{
+          {1.0, +0x1p-1074},
+          {1.0, -0x1p-1074},
+          APFloat::cmpGreaterThan,
+      },
+  };
+  return CompareAbsoluteValueTestCases;
+}
+} // namespace PPCDoubleDoubleCompareAbsoluteValueTestDetails
+
+class PPCDoubleDoubleCompareAbsoluteValueValueTest
+    : public testing::Test,
+      public ::testing::WithParamInterface<
+          PPCDoubleDoubleCompareAbsoluteValueTestDetails::TestCase> {};
+
+INSTANTIATE_TEST_SUITE_P(
+    PPCDoubleDoubleCompareAbsoluteValueValueParamTests,
+    PPCDoubleDoubleCompareAbsoluteValueValueTest,
+    ::testing::ValuesIn(
+        PPCDoubleDoubleCompareAbsoluteValueTestDetails::testCases()));
+
+TEST_P(PPCDoubleDoubleCompareAbsoluteValueValueTest,
+       PPCDoubleDoubleCompareAbsoluteValue) {
+  auto Param = GetParam();
+  for (bool LHSNegate : {false, true}) {
+    auto LHS = llvm::detail::DoubleAPFloat{APFloat::PPCDoubleDouble(),
+                                           APFloat{Param.LHS.Hi},
+                                           APFloat{Param.LHS.Lo}};
+    if (LHSNegate)
+      LHS.changeSign();
+    for (bool RHSNegate : {false, true}) {
+      auto RHS = llvm::detail::DoubleAPFloat{APFloat::PPCDoubleDouble(),
+                                             APFloat{Param.RHS.Hi},
+                                             APFloat{Param.RHS.Lo}};
+      if (RHSNegate)
+        RHS.changeSign();
+
+      EXPECT_EQ(LHS.compareAbsoluteValue(RHS), Param.Result);
+    }
+  }
+}
+
 TEST(APFloatTest, PPCDoubleDoubleBitwiseIsEqual) {
   using DataType = std::tuple<uint64_t, uint64_t, uint64_t, uint64_t, bool>;