Skip to content

Commit 149f5b5

Browse files
rotaterightrotateright
committed
[APFloat] convert SNaN to QNaN in convert() and raise Invalid signal
This is an alternate fix (see D87835) for a bug where a NaN constant gets wrongly transformed into Infinity via truncation. In this patch, we uniformly convert any SNaN to QNaN while raising 'invalid op'. But we don't have a way to directly specify a 32-bit SNaN value in LLVM IR, so those are always encoded/decoded by calling convert from/to 64-bit hex. See D88664 for a clang fix needed to allow this change. Differential Revision: https://reviews.llvm.org/D88238
1 parent 499260c commit 149f5b5

File tree

9 files changed

+80
-40
lines changed

9 files changed

+80
-40
lines changed

‎clang/test/CodeGen/builtin-nan-exception.c

+5-1
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,12 @@ float f[] = {
1717

1818

1919
// Doubles are created and converted to floats.
20+
// Converting (truncating) to float quiets the NaN (sets the MSB
21+
// of the significand) and raises the APFloat invalidOp exception
22+
// but that should not cause a compilation error in the default
23+
// (ignore FP exceptions) mode.
2024

21-
// CHECK: float 0x7FF8000000000000, float 0x7FF4000000000000
25+
// CHECK: float 0x7FF8000000000000, float 0x7FFC000000000000
2226

2327
float converted_to_float[] = {
2428
__builtin_nan(""),

‎clang/test/CodeGen/builtin-nan-legacy.c

+9-1
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,15 @@
11
// RUN: %clang -target mipsel-unknown-linux -mnan=legacy -emit-llvm -S %s -o - | FileCheck %s
2-
// CHECK: float 0x7FF4000000000000, float 0x7FF8000000000000
2+
// CHECK: float 0x7FFC000000000000, float 0x7FF8000000000000
33
// CHECK: double 0x7FF4000000000000, double 0x7FF8000000000000
44

5+
// The first line shows an unintended consequence.
6+
// __builtin_nan() creates a legacy QNAN double with an empty payload
7+
// (the first bit of the significand is clear to indicate quiet, so
8+
// the second bit of the payload is set to maintain NAN-ness).
9+
// The value is then truncated, but llvm::APFloat does not know about
10+
// the inverted quiet bit, so it sets the first bit on conversion
11+
// to indicate 'quiet' independently of the setting in clang.
12+
513
float f[] = {
614
__builtin_nan(""),
715
__builtin_nans(""),

‎clang/test/CodeGen/mips-unsupported-nan.c

+15-1
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,21 @@
3939
// CHECK-MIPS64: warning: ignoring '-mnan=2008' option because the 'mips64' architecture does not support it
4040
// CHECK-MIPS64R6: warning: ignoring '-mnan=legacy' option because the 'mips64r6' architecture does not support it
4141

42-
// CHECK-NANLEGACY: float 0x7FF4000000000000
42+
// This call creates a QNAN double with an empty payload.
43+
// The quiet bit is inverted in legacy mode: it is clear to indicate QNAN,
44+
// so the next highest bit is set to maintain NAN (not infinity).
45+
// In regular (2008) mode, the quiet bit is set to indicate QNAN.
46+
47+
// CHECK-NANLEGACY: double 0x7FF4000000000000
48+
// CHECK-NAN2008: double 0x7FF8000000000000
49+
50+
double d = __builtin_nan("");
51+
52+
// This call creates a QNAN double with an empty payload and then truncates.
53+
// llvm::APFloat does not know about the inverted quiet bit, so it sets the
54+
// quiet bit on conversion independently of the setting in clang.
55+
56+
// CHECK-NANLEGACY: float 0x7FFC000000000000
4357
// CHECK-NAN2008: float 0x7FF8000000000000
4458

4559
float f = __builtin_nan("");

‎llvm/lib/AsmParser/LLParser.cpp

+10
Original file line numberDiff line numberDiff line change
@@ -5345,6 +5345,8 @@ bool LLParser::ConvertValIDToValue(Type *Ty, ValID &ID, Value *&V,
53455345
// The lexer has no type info, so builds all half, bfloat, float, and double
53465346
// FP constants as double. Fix this here. Long double does not need this.
53475347
if (&ID.APFloatVal.getSemantics() == &APFloat::IEEEdouble()) {
5348+
// Check for signaling before potentially converting and losing that info.
5349+
bool IsSNAN = ID.APFloatVal.isSignaling();
53485350
bool Ignored;
53495351
if (Ty->isHalfTy())
53505352
ID.APFloatVal.convert(APFloat::IEEEhalf(), APFloat::rmNearestTiesToEven,
@@ -5355,6 +5357,14 @@ bool LLParser::ConvertValIDToValue(Type *Ty, ValID &ID, Value *&V,
53555357
else if (Ty->isFloatTy())
53565358
ID.APFloatVal.convert(APFloat::IEEEsingle(), APFloat::rmNearestTiesToEven,
53575359
&Ignored);
5360+
if (IsSNAN) {
5361+
// The convert call above may quiet an SNaN, so manufacture another
5362+
// SNaN. The bitcast works because the payload (significand) parameter
5363+
// is truncated to fit.
5364+
APInt Payload = ID.APFloatVal.bitcastToAPInt();
5365+
ID.APFloatVal = APFloat::getSNaN(ID.APFloatVal.getSemantics(),
5366+
ID.APFloatVal.isNegative(), &Payload);
5367+
}
53585368
}
53595369
V = ConstantFP::get(Context, ID.APFloatVal);
53605370

‎llvm/lib/IR/AsmWriter.cpp

+12-2
Original file line numberDiff line numberDiff line change
@@ -1373,9 +1373,19 @@ static void WriteConstantInternal(raw_ostream &Out, const Constant *CV,
13731373
"assuming that double is 64 bits!");
13741374
APFloat apf = APF;
13751375
// Floats are represented in ASCII IR as double, convert.
1376-
if (!isDouble)
1376+
// FIXME: We should allow 32-bit hex float and remove this.
1377+
if (!isDouble) {
1378+
// A signaling NaN is quieted on conversion, so we need to recreate the
1379+
// expected value after convert (quiet bit of the payload is clear).
1380+
bool IsSNAN = apf.isSignaling();
13771381
apf.convert(APFloat::IEEEdouble(), APFloat::rmNearestTiesToEven,
1378-
&ignored);
1382+
&ignored);
1383+
if (IsSNAN) {
1384+
APInt Payload = apf.bitcastToAPInt();
1385+
apf = APFloat::getSNaN(APFloat::IEEEdouble(), apf.isNegative(),
1386+
&Payload);
1387+
}
1388+
}
13791389
Out << format_hex(apf.bitcastToAPInt().getZExtValue(), 0, /*Upper=*/true);
13801390
return;
13811391
}

‎llvm/lib/Support/APFloat.cpp

+8-19
Original file line numberDiff line numberDiff line change
@@ -2243,26 +2243,15 @@ IEEEFloat::opStatus IEEEFloat::convert(const fltSemantics &toSemantics,
22432243
if (!X86SpecialNan && semantics == &semX87DoubleExtended)
22442244
APInt::tcSetBit(significandParts(), semantics->precision - 1);
22452245

2246-
// If we are truncating NaN, it is possible that we shifted out all of the
2247-
// set bits in a signalling NaN payload. But NaN must remain NaN, so some
2248-
// bit in the significand must be set (otherwise it is Inf).
2249-
// This can only happen with sNaN. Set the 1st bit after the quiet bit,
2250-
// so that we still have an sNaN.
2251-
// FIXME: Set quiet and return opInvalidOp (on convert of any sNaN).
2252-
// But this requires fixing LLVM to parse 32-bit hex FP or ignoring
2253-
// conversions while parsing IR.
2254-
if (APInt::tcIsZero(significandParts(), newPartCount)) {
2255-
assert(shift < 0 && "Should not lose NaN payload on extend");
2256-
assert(semantics->precision >= 3 && "Unexpectedly narrow significand");
2257-
assert(*losesInfo && "Missing payload should have set lost info");
2258-
APInt::tcSetBit(significandParts(), semantics->precision - 3);
2246+
// Convert of sNaN creates qNaN and raises an exception (invalid op).
2247+
// This also guarantees that a sNaN does not become Inf on a truncation
2248+
// that loses all payload bits.
2249+
if (isSignaling()) {
2250+
makeQuiet();
2251+
fs = opInvalidOp;
2252+
} else {
2253+
fs = opOK;
22592254
}
2260-
2261-
// gcc forces the Quiet bit on, which means (float)(double)(float_sNan)
2262-
// does not give you back the same bits. This is dubious, and we
2263-
// don't currently do it. You're really supposed to get
2264-
// an invalid operation signal at runtime, but nobody does that.
2265-
fs = opOK;
22662255
} else {
22672256
*losesInfo = false;
22682257
fs = opOK;

‎llvm/test/Transforms/InstSimplify/ConstProp/cast.ll

+4-4
Original file line numberDiff line numberDiff line change
@@ -40,24 +40,24 @@ define float @overflow_sitofp() {
4040
}
4141

4242
; https://llvm.org/PR43907 - make sure that NaN doesn't morph into Inf.
43-
; SNaN remains SNaN.
43+
; SNaN becomes QNaN.
4444

4545
define float @nan_f64_trunc() {
4646
; CHECK-LABEL: @nan_f64_trunc(
47-
; CHECK-NEXT: ret float 0x7FF4000000000000
47+
; CHECK-NEXT: ret float 0x7FF8000000000000
4848
;
4949
%f = fptrunc double 0x7FF0000000000001 to float
5050
ret float %f
5151
}
5252

5353
; Verify again with a vector and different destination type.
54-
; SNaN remains SNaN (first two elements).
54+
; SNaN becomes SNaN (first two elements).
5555
; QNaN remains QNaN (third element).
5656
; Lower 42 bits of NaN source payload are lost.
5757

5858
define <3 x half> @nan_v3f64_trunc() {
5959
; CHECK-LABEL: @nan_v3f64_trunc(
60-
; CHECK-NEXT: ret <3 x half> <half 0xH7D00, half 0xH7D00, half 0xH7E00>
60+
; CHECK-NEXT: ret <3 x half> <half 0xH7E00, half 0xH7E00, half 0xH7E00>
6161
;
6262
%f = fptrunc <3 x double> <double 0x7FF0020000000000, double 0x7FF003FFFFFFFFFF, double 0x7FF8000000000001> to <3 x half>
6363
ret <3 x half> %f

‎llvm/test/Transforms/PhaseOrdering/X86/nancvt.ll

+9-6
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@ target triple = "i686-apple-darwin8"
1818

1919
@var = external global i32
2020

21+
; SNAN becomes QNAN on fptrunc:
22+
; 2147228864 = 0x7ffc1cc0 : QNAN
23+
2124
define i32 @main() {
2225
; CHECK-LABEL: @main(
2326
; CHECK-NEXT: entry:
@@ -30,15 +33,15 @@ define i32 @main() {
3033
; CHECK-NEXT: store volatile i32 2147228864, i32* @var, align 4
3134
; CHECK-NEXT: store volatile i32 2147228864, i32* @var, align 4
3235
; CHECK-NEXT: store volatile i32 2147228864, i32* @var, align 4
33-
; CHECK-NEXT: store volatile i32 2146502828, i32* @var, align 4
36+
; CHECK-NEXT: store volatile i32 2147027116, i32* @var, align 4
3437
; CHECK-NEXT: store volatile i32 -1610612736, i32* @var, align 4
35-
; CHECK-NEXT: store volatile i32 2146502828, i32* @var, align 4
38+
; CHECK-NEXT: store volatile i32 2147027116, i32* @var, align 4
3639
; CHECK-NEXT: store volatile i32 -2147483648, i32* @var, align 4
37-
; CHECK-NEXT: store volatile i32 2146502828, i32* @var, align 4
40+
; CHECK-NEXT: store volatile i32 2147027116, i32* @var, align 4
3841
; CHECK-NEXT: store volatile i32 -1073741824, i32* @var, align 4
39-
; CHECK-NEXT: store volatile i32 2143034560, i32* @var, align 4
40-
; CHECK-NEXT: store volatile i32 2143034560, i32* @var, align 4
41-
; CHECK-NEXT: store volatile i32 2143034560, i32* @var, align 4
42+
; CHECK-NEXT: store volatile i32 2147228864, i32* @var, align 4
43+
; CHECK-NEXT: store volatile i32 2147228864, i32* @var, align 4
44+
; CHECK-NEXT: store volatile i32 2147228864, i32* @var, align 4
4245
; CHECK-NEXT: ret i32 undef
4346
;
4447
entry:

‎llvm/unittests/ADT/APFloatTest.cpp

+8-6
Original file line numberDiff line numberDiff line change
@@ -1816,11 +1816,12 @@ TEST(APFloatTest, convert) {
18161816
EXPECT_FALSE(losesInfo);
18171817

18181818
test = APFloat::getSNaN(APFloat::IEEEsingle());
1819-
APFloat X87SNaN = APFloat::getSNaN(APFloat::x87DoubleExtended());
18201819
APFloat::opStatus status = test.convert(APFloat::x87DoubleExtended(), APFloat::rmNearestTiesToEven, &losesInfo);
1821-
EXPECT_TRUE(test.bitwiseIsEqual(X87SNaN));
1820+
// Conversion quiets the SNAN, so now 2 bits of the 64-bit significand should be set.
1821+
APInt topTwoBits(64, 0x6000000000000000);
1822+
EXPECT_TRUE(test.bitwiseIsEqual(APFloat::getQNaN(APFloat::x87DoubleExtended(), false, &topTwoBits)));
18221823
EXPECT_FALSE(losesInfo);
1823-
EXPECT_EQ(status, APFloat::opOK);
1824+
EXPECT_EQ(status, APFloat::opInvalidOp);
18241825

18251826
test = APFloat::getQNaN(APFloat::IEEEsingle());
18261827
APFloat X87QNaN = APFloat::getQNaN(APFloat::x87DoubleExtended());
@@ -1832,6 +1833,7 @@ TEST(APFloatTest, convert) {
18321833
test = APFloat::getSNaN(APFloat::x87DoubleExtended());
18331834
test.convert(APFloat::x87DoubleExtended(), APFloat::rmNearestTiesToEven,
18341835
&losesInfo);
1836+
APFloat X87SNaN = APFloat::getSNaN(APFloat::x87DoubleExtended());
18351837
EXPECT_TRUE(test.bitwiseIsEqual(X87SNaN));
18361838
EXPECT_FALSE(losesInfo);
18371839

@@ -1841,13 +1843,13 @@ TEST(APFloatTest, convert) {
18411843
EXPECT_TRUE(test.bitwiseIsEqual(X87QNaN));
18421844
EXPECT_FALSE(losesInfo);
18431845

1844-
// The payload is lost in truncation, but we must retain NaN, so we set the bit after the quiet bit.
1846+
// The payload is lost in truncation, but we retain NaN by setting the quiet bit.
18451847
APInt payload(52, 1);
18461848
test = APFloat::getSNaN(APFloat::IEEEdouble(), false, &payload);
18471849
status = test.convert(APFloat::IEEEsingle(), APFloat::rmNearestTiesToEven, &losesInfo);
1848-
EXPECT_EQ(0x7fa00000, test.bitcastToAPInt());
1850+
EXPECT_EQ(0x7fc00000, test.bitcastToAPInt());
18491851
EXPECT_TRUE(losesInfo);
1850-
EXPECT_EQ(status, APFloat::opOK);
1852+
EXPECT_EQ(status, APFloat::opInvalidOp);
18511853

18521854
// The payload is lost in truncation. QNaN remains QNaN.
18531855
test = APFloat::getQNaN(APFloat::IEEEdouble(), false, &payload);

0 commit comments

Comments
 (0)