Skip to content

Commit

Permalink
[APFloat] prevent NaN morphing into Inf on conversion (PR43907)
Browse files Browse the repository at this point in the history
We shift the significand right on a truncation, but that needs to be made NaN-safe:
always set at least 1 bit in the significand.
https://llvm.org/PR43907

See D88238 for the likely follow-up (but needs some plumbing fixes before it can proceed).

Differential Revision: https://reviews.llvm.org/D87835

(cherry picked from commit e34bd1e)
  • Loading branch information
rotateright authored and zmodem committed Sep 30, 2020
1 parent a3aee26 commit 60a2520
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 0 deletions.
15 changes: 15 additions & 0 deletions llvm/lib/Support/APFloat.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2242,6 +2242,21 @@ IEEEFloat::opStatus IEEEFloat::convert(const fltSemantics &toSemantics,
if (!X86SpecialNan && semantics == &semX87DoubleExtended)
APInt::tcSetBit(significandParts(), semantics->precision - 1);

// If we are truncating NaN, it is possible that we shifted out all of the
// set bits in a signalling NaN payload. But NaN must remain NaN, so some
// bit in the significand must be set (otherwise it is Inf).
// This can only happen with sNaN. Set the 1st bit after the quiet bit,
// so that we still have an sNaN.
// FIXME: Set quiet and return opInvalidOp (on convert of any sNaN).
// But this requires fixing LLVM to parse 32-bit hex FP or ignoring
// conversions while parsing IR.
if (APInt::tcIsZero(significandParts(), newPartCount)) {
assert(shift < 0 && "Should not lose NaN payload on extend");
assert(semantics->precision >= 3 && "Unexpectedly narrow significand");
assert(*losesInfo && "Missing payload should have set lost info");
APInt::tcSetBit(significandParts(), semantics->precision - 3);
}

// gcc forces the Quiet bit on, which means (float)(double)(float_sNan)
// does not give you back the same bits. This is dubious, and we
// don't currently do it. You're really supposed to get
Expand Down
23 changes: 23 additions & 0 deletions llvm/test/Transforms/ConstProp/cast.ll
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,26 @@ define float @overflow_sitofp() {
ret float %i
}

; https://llvm.org/PR43907 - make sure that NaN doesn't morph into Inf.
; SNaN remains SNaN.

define float @nan_f64_trunc() {
; CHECK-LABEL: @nan_f64_trunc(
; CHECK-NEXT: ret float 0x7FF4000000000000
;
%f = fptrunc double 0x7FF0000000000001 to float
ret float %f
}

; Verify again with a vector and different destination type.
; SNaN remains SNaN (first two elements).
; QNaN remains QNaN (third element).
; Lower 42 bits of NaN source payload are lost.

define <3 x half> @nan_v3f64_trunc() {
; CHECK-LABEL: @nan_v3f64_trunc(
; CHECK-NEXT: ret <3 x half> <half 0xH7D00, half 0xH7D00, half 0xH7E00>
;
%f = fptrunc <3 x double> <double 0x7FF0020000000000, double 0x7FF003FFFFFFFFFF, double 0x7FF8000000000001> to <3 x half>
ret <3 x half> %f
}
15 changes: 15 additions & 0 deletions llvm/unittests/ADT/APFloatTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1840,6 +1840,21 @@ TEST(APFloatTest, convert) {
&losesInfo);
EXPECT_TRUE(test.bitwiseIsEqual(X87QNaN));
EXPECT_FALSE(losesInfo);

// The payload is lost in truncation, but we must retain NaN, so we set the bit after the quiet bit.
APInt payload(52, 1);
test = APFloat::getSNaN(APFloat::IEEEdouble(), false, &payload);
APFloat::opStatus status = test.convert(APFloat::IEEEsingle(), APFloat::rmNearestTiesToEven, &losesInfo);
EXPECT_EQ(0x7fa00000, test.bitcastToAPInt());
EXPECT_TRUE(losesInfo);
EXPECT_EQ(status, APFloat::opOK);

// The payload is lost in truncation. QNaN remains QNaN.
test = APFloat::getQNaN(APFloat::IEEEdouble(), false, &payload);
status = test.convert(APFloat::IEEEsingle(), APFloat::rmNearestTiesToEven, &losesInfo);
EXPECT_EQ(0x7fc00000, test.bitcastToAPInt());
EXPECT_TRUE(losesInfo);
EXPECT_EQ(status, APFloat::opOK);
}

TEST(APFloatTest, PPCDoubleDouble) {
Expand Down

0 comments on commit 60a2520

Please sign in to comment.