[libc][math][c23] Fix X86_Binary80 special cases for canonicalize functions.

[Sh0g0-1758]

Updates the special case of pseudo infinty as pointed in #85940

[Sh0g0-1758]

cc: @jcranmer-intel

[llvmbot]

@llvm/pr-subscribers-libc

Author: Shourya Goel (Sh0g0-1758)

Changes

Updates the special case of pseudo infinty as pointed in #85940

---

Full diff: https://github.com/llvm/llvm-project/pull/86924.diff

2 Files Affected:

• (modified) libc/src/__support/FPUtil/BasicOperations.h (+6-4)
• (modified) libc/test/src/math/smoke/CanonicalizeTest.h (+3-3)

diff --git a/libc/src/__support/FPUtil/BasicOperations.h b/libc/src/__support/FPUtil/BasicOperations.h
index a47931bb33900a..bdb91a98a0d357 100644
--- a/libc/src/__support/FPUtil/BasicOperations.h
+++ b/libc/src/__support/FPUtil/BasicOperations.h
@@ -185,7 +185,7 @@ LIBC_INLINE int canonicalize(T &cx, const T &x) {
     // More precisely :
     // Exponent   |       Significand      | Meaning
     //            | Bits 63-62 | Bits 61-0 |
-    // All Ones   |     00     |    Zero   | Pseudo Infinity, Value = Infinty
+    // All Ones   |     00     |    Zero   | Pseudo Infinity, Value = SNaN
     // All Ones   |     00     |  Non-Zero | Pseudo NaN, Value = SNaN
     // All Ones   |     01     | Anything  | Pseudo NaN, Value = SNaN
     //            |   Bit 63   | Bits 62-0 |
@@ -199,9 +199,11 @@ LIBC_INLINE int canonicalize(T &cx, const T &x) {
     int exponent = sx.get_biased_exponent();
     if (exponent == 0x7FFF) {
       if (!bit63 && !bit62) {
-        if (mantissa == 0)
-          cx = FPBits<T>::inf(sx.sign()).get_val();
-        else {
+        if (mantissa == 0) {
+          cx = FPBits<T>::quiet_nan(sx.sign(), mantissa).get_val();
+          raise_except_if_required(FE_INVALID);
+          return 1;
+        } else {
           cx = FPBits<T>::quiet_nan(sx.sign(), mantissa).get_val();
           raise_except_if_required(FE_INVALID);
           return 1;
diff --git a/libc/test/src/math/smoke/CanonicalizeTest.h b/libc/test/src/math/smoke/CanonicalizeTest.h
index c8af83f1983881..1891d47922cf88 100644
--- a/libc/test/src/math/smoke/CanonicalizeTest.h
+++ b/libc/test/src/math/smoke/CanonicalizeTest.h
@@ -55,12 +55,12 @@ class CanonicalizeTest : public LIBC_NAMESPACE::testing::Test {
       T cx;
       // Exponent   |       Significand      | Meaning
       //            | Bits 63-62 | Bits 61-0 |
-      // All Ones   |     00     |    Zero   | Pseudo Infinity, Value = Infinty
+      // All Ones   |     00     |    Zero   | Pseudo Infinity, Value = SNaN
 
       FPBits test1((UInt128(0x7FFF) << 64) + UInt128(0x0000000000000000));
       const T test1_val = test1.get_val();
-      TEST_SPECIAL(cx, test1_val, 0, 0);
-      EXPECT_FP_EQ(cx, inf);
+      TEST_SPECIAL(cx, test1_val, 1, FE_INVALID);
+      EXPECT_FP_EQ(cx, aNaN);
 
       // Exponent   |       Significand      | Meaning
       //            | Bits 63-62 | Bits 61-0 |

[nickdesaulniers]

Do we only need to do this for x86 or all targets?

[jcranmer-intel]

Unnormals also need changing.

[Sh0g0-1758]

@nickdesaulniers, I don't think I follow you. Rest of the targets are already in canonical form, no?

@jcranmer-intel, updated Unnormal too, now they return a qNAN with FE_INVALID

[Sh0g0-1758]

Fixed tests. Ran into a rather interesting observation. Why is that

A = (UInt128(0x0000) << 64) + UInt128(0x8000000000000000)
B = (UInt128(0x0001) << 64) + UInt128(0x8000000000000000)

Both have a get_biased_exponent() value of 1 ?

[lntue]

Fixed tests. Ran into a rather interesting observation. Why is that

A = (UInt128(0x0000) << 64) + UInt128(0x8000000000000000)
B = (UInt128(0x0001) << 64) + UInt128(0x8000000000000000)

Both have a get_biased_exponent() value of 1 ?

get_biased_exponent follows the IEEE 754 convention, and is used to compute the real values of the bit patterns. So once you added a hidden bit to your mantissa, the real value is simply

  2^(get_biased_exponent() - EXPONENT_BIAS) * m.

Another way to think about it is that:

  2^(get_biased_exponent() - EXPONENT_BIAS - FRACTION_LEN) is the value of the least significant bit.

[Sh0g0-1758]

Fixed tests. Ran into a rather interesting observation. Why is that

A = (UInt128(0x0000) << 64) + UInt128(0x8000000000000000)
B = (UInt128(0x0001) << 64) + UInt128(0x8000000000000000)

Both have a get_biased_exponent() value of 1 ?

get_biased_exponent follows the IEEE 754 convention, and is used to compute the real values of the bit patterns. So once you added a hidden bit to your mantissa, the real value is simply

  2^(get_biased_exponent() - EXPONENT_BIAS) * m.

Another way to think about it is that:

  2^(get_biased_exponent() - EXPONENT_BIAS - FRACTION_LEN) is the value of the least significant bit.

Thanks for the explanation. Actually replacing the tests with _u128 strings gave me added clarity as I played around with the bits a lot while locally testing. Thanks for the suggestion.

[jcranmer-intel]

The logic here looks correct now to me; I'll leave it to the libc maintainers to make other commentary on the code.

[nickdesaulniers]

Failing in postsubmit:
https://lab.llvm.org/buildbot/#/builders/90/builds/67461/steps/7/logs/stdio

[nickdesaulniers]

Sorry, it's not clear to me why this is failing. I've reverted it for now in d357324.

Fails in presubmit.

Should have been "postsubmit." Oops.

[lntue]

I ran it with -DLLVM_USE_SANITIZER=Undefined and got:

[ RUN      ] LlvmLibcCanonicalizeTest.X64_80SpecialNumbers
/usr/local/google/home/lntue/experiment/llvm/llvm-project/libc/src/__support/integer_literals.h:102:7: runtime error: execution reached an unreachable program point
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/local/google/home/lntue/experiment/llvm/llvm-project/libc/src/__support/integer_literals.h:102:7 in 

https://github.com/llvm/llvm-project/blob/main/libc/src/__support/integer_literals.h#L102

[lntue]

I think it is because the first groups of your uint128 literals have more than 8 0's on L70,75,80,85.

[Sh0g0-1758]

@nickdesaulniers, as pointed out by @intue, the error was caused by an extra 0 in the tests. I have linked a patch for it. Please land the changes on main so that the patch can land.

[nickdesaulniers]

No, we don't reland broken changes in main in llvm-project. Add a revert of d357324 in your PR #87016 first, with the fix either as a distinct reviewer which is squashed when merging (a nice touch for reviewers to see what's new in the reland), or just have 1 commit which is this PR + the fixup squashed (less easy for reviewers to see what's different).

[Sh0g0-1758]

@nickdesaulniers, to avoid mutch hassle, I have opened a fresh PR with the changes from this PR and the updated test cases.