Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APFloat: x87DoubleExtended pseudo-NaNs (integer_bit==0) not handled as always-signalling. #63938

Open
eddyb opened this issue Jul 18, 2023 · 0 comments
Labels
floating-point Floating-point math llvm:core

Comments

@eddyb
Copy link
Contributor

eddyb commented Jul 18, 2023

Based on https://reviews.llvm.org/D41868 (cc @labath @stephentyrone) and these snippets of APFloat.cpp:

/// Integer bit is explicit in this format. Intel hardware (387 and later)
/// does not support these bit patterns:
/// exponent = all 1's, integer bit 0, significand 0 ("pseudoinfinity")
/// exponent = all 1's, integer bit 0, significand nonzero ("pseudoNaN")
/// exponent!=0 nor all 1's, integer bit 0 ("unnormal")
/// exponent = 0, integer bit 1 ("pseudodenormal")
/// At the moment, the first three are treated as NaNs, the last one as Normal.

// For x87 extended precision, we want to make a NaN, not a
// pseudo-NaN. Maybe we should expose the ability to make
// pseudo-NaNs?
if (semantics == &semX87DoubleExtended)
APInt::tcSetBit(significand, QNaNBit + 1);

I would expect the following:

  • pseudo-NaNs should always be treated as SNaNs
    • this would presumably also apply to pseudo-infinities and unnormals (all of which have in common integer_bit == 0 w/o being subnormals, AIUI)
    • not the case today: isSignaling ignores the integer bit and only checks the quiet bit (which a pseudo-NaN can set)
  • quieting a pseudo-NaN should produce a legal QNaN (while setting the opInvalidOp status)
    • that is, makeQuiet should replicate makeNaN's APInt::tcSetBit(significand, QNaNBit + 1)
    • not the case today: makeQuiet fails to set the integer bit, potentially leaving a pseudo-NaN behind

Or in other words, I'd expect this test to pass (currently it fails to even generate an opInvalidOp status):

TEST(APFloatTest, x87PseudoNaN) {
  APFloat PseudoNaN(APFloat::x87DoubleExtended(), APInt(80, {1ULL << 62, 0x7fff}));
  EXPECT_TRUE(PseudoNaN.isNaN());
  EXPECT_TRUE(PseudoNaN.isSignaling()); // FAIL: pseudo-NaNs aren't detected as SNaNs

  // Any operation (which propagates input SNaNs while quieting them),
  // `roundToIntegral` just happened to have similar tests already.
  APFloat test = PseudoNaN;
  APFloat::opStatus St = test.roundToIntegral(APFloat::rmTowardZero);
  EXPECT_TRUE(test.isNaN());
  EXPECT_FALSE(test.isSignaling()); // would FAIL if pseudo-NaNs were detected as SNaNs,
                                    // and `makeQuiet` wasn't *also* updated
  EXPECT_FALSE(test.isNegative());
  EXPECT_EQ(APFloat::opInvalidOp, St); // FAIL: pseudo-NaNs aren't detected as SNaNs
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
floating-point Floating-point math llvm:core
Projects
None yet
Development

No branches or pull requests

2 participants