[clang] 149f5b5 - [APFloat] convert SNaN to QNaN in convert() and raise Invalid signal
Sanjay Patel via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 1 11:37:50 PDT 2020
Author: Sanjay Patel
Date: 2020-10-01T14:37:38-04:00
New Revision: 149f5b573c79eac0c519ada4d2f7c50e17796cdf
URL: https://github.com/llvm/llvm-project/commit/149f5b573c79eac0c519ada4d2f7c50e17796cdf
DIFF: https://github.com/llvm/llvm-project/commit/149f5b573c79eac0c519ada4d2f7c50e17796cdf.diff
LOG: [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
Added:
Modified:
clang/test/CodeGen/builtin-nan-exception.c
clang/test/CodeGen/builtin-nan-legacy.c
clang/test/CodeGen/mips-unsupported-nan.c
llvm/lib/AsmParser/LLParser.cpp
llvm/lib/IR/AsmWriter.cpp
llvm/lib/Support/APFloat.cpp
llvm/test/Transforms/InstSimplify/ConstProp/cast.ll
llvm/test/Transforms/PhaseOrdering/X86/nancvt.ll
llvm/unittests/ADT/APFloatTest.cpp
Removed:
################################################################################
diff --git a/clang/test/CodeGen/builtin-nan-exception.c b/clang/test/CodeGen/builtin-nan-exception.c
index a0de25e52ebe6..7445411ddf89e 100644
--- a/clang/test/CodeGen/builtin-nan-exception.c
+++ b/clang/test/CodeGen/builtin-nan-exception.c
@@ -17,8 +17,12 @@ float f[] = {
// Doubles are created and converted to floats.
+// Converting (truncating) to float quiets the NaN (sets the MSB
+// of the significand) and raises the APFloat invalidOp exception
+// but that should not cause a compilation error in the default
+// (ignore FP exceptions) mode.
-// CHECK: float 0x7FF8000000000000, float 0x7FF4000000000000
+// CHECK: float 0x7FF8000000000000, float 0x7FFC000000000000
float converted_to_float[] = {
__builtin_nan(""),
diff --git a/clang/test/CodeGen/builtin-nan-legacy.c b/clang/test/CodeGen/builtin-nan-legacy.c
index cd0f0fd14f14c..de6c15379a4dd 100644
--- a/clang/test/CodeGen/builtin-nan-legacy.c
+++ b/clang/test/CodeGen/builtin-nan-legacy.c
@@ -1,7 +1,15 @@
// RUN: %clang -target mipsel-unknown-linux -mnan=legacy -emit-llvm -S %s -o - | FileCheck %s
-// CHECK: float 0x7FF4000000000000, float 0x7FF8000000000000
+// CHECK: float 0x7FFC000000000000, float 0x7FF8000000000000
// CHECK: double 0x7FF4000000000000, double 0x7FF8000000000000
+// The first line shows an unintended consequence.
+// __builtin_nan() creates a legacy QNAN double with an empty payload
+// (the first bit of the significand is clear to indicate quiet, so
+// the second bit of the payload is set to maintain NAN-ness).
+// The value is then truncated, but llvm::APFloat does not know about
+// the inverted quiet bit, so it sets the first bit on conversion
+// to indicate 'quiet' independently of the setting in clang.
+
float f[] = {
__builtin_nan(""),
__builtin_nans(""),
diff --git a/clang/test/CodeGen/mips-unsupported-nan.c b/clang/test/CodeGen/mips-unsupported-nan.c
index 2fd5042e92f8e..16cea3c2e7e18 100644
--- a/clang/test/CodeGen/mips-unsupported-nan.c
+++ b/clang/test/CodeGen/mips-unsupported-nan.c
@@ -39,7 +39,21 @@
// CHECK-MIPS64: warning: ignoring '-mnan=2008' option because the 'mips64' architecture does not support it
// CHECK-MIPS64R6: warning: ignoring '-mnan=legacy' option because the 'mips64r6' architecture does not support it
-// CHECK-NANLEGACY: float 0x7FF4000000000000
+// This call creates a QNAN double with an empty payload.
+// The quiet bit is inverted in legacy mode: it is clear to indicate QNAN,
+// so the next highest bit is set to maintain NAN (not infinity).
+// In regular (2008) mode, the quiet bit is set to indicate QNAN.
+
+// CHECK-NANLEGACY: double 0x7FF4000000000000
+// CHECK-NAN2008: double 0x7FF8000000000000
+
+double d = __builtin_nan("");
+
+// This call creates a QNAN double with an empty payload and then truncates.
+// llvm::APFloat does not know about the inverted quiet bit, so it sets the
+// quiet bit on conversion independently of the setting in clang.
+
+// CHECK-NANLEGACY: float 0x7FFC000000000000
// CHECK-NAN2008: float 0x7FF8000000000000
float f = __builtin_nan("");
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index 63f8531dbdced..4e1ae4faa4e19 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -5345,6 +5345,8 @@ bool LLParser::ConvertValIDToValue(Type *Ty, ValID &ID, Value *&V,
// The lexer has no type info, so builds all half, bfloat, float, and double
// FP constants as double. Fix this here. Long double does not need this.
if (&ID.APFloatVal.getSemantics() == &APFloat::IEEEdouble()) {
+ // Check for signaling before potentially converting and losing that info.
+ bool IsSNAN = ID.APFloatVal.isSignaling();
bool Ignored;
if (Ty->isHalfTy())
ID.APFloatVal.convert(APFloat::IEEEhalf(), APFloat::rmNearestTiesToEven,
@@ -5355,6 +5357,14 @@ bool LLParser::ConvertValIDToValue(Type *Ty, ValID &ID, Value *&V,
else if (Ty->isFloatTy())
ID.APFloatVal.convert(APFloat::IEEEsingle(), APFloat::rmNearestTiesToEven,
&Ignored);
+ if (IsSNAN) {
+ // The convert call above may quiet an SNaN, so manufacture another
+ // SNaN. The bitcast works because the payload (significand) parameter
+ // is truncated to fit.
+ APInt Payload = ID.APFloatVal.bitcastToAPInt();
+ ID.APFloatVal = APFloat::getSNaN(ID.APFloatVal.getSemantics(),
+ ID.APFloatVal.isNegative(), &Payload);
+ }
}
V = ConstantFP::get(Context, ID.APFloatVal);
diff --git a/llvm/lib/IR/AsmWriter.cpp b/llvm/lib/IR/AsmWriter.cpp
index 8cb1883da68e4..550aa1395bef3 100644
--- a/llvm/lib/IR/AsmWriter.cpp
+++ b/llvm/lib/IR/AsmWriter.cpp
@@ -1373,9 +1373,19 @@ static void WriteConstantInternal(raw_ostream &Out, const Constant *CV,
"assuming that double is 64 bits!");
APFloat apf = APF;
// Floats are represented in ASCII IR as double, convert.
- if (!isDouble)
+ // FIXME: We should allow 32-bit hex float and remove this.
+ if (!isDouble) {
+ // A signaling NaN is quieted on conversion, so we need to recreate the
+ // expected value after convert (quiet bit of the payload is clear).
+ bool IsSNAN = apf.isSignaling();
apf.convert(APFloat::IEEEdouble(), APFloat::rmNearestTiesToEven,
- &ignored);
+ &ignored);
+ if (IsSNAN) {
+ APInt Payload = apf.bitcastToAPInt();
+ apf = APFloat::getSNaN(APFloat::IEEEdouble(), apf.isNegative(),
+ &Payload);
+ }
+ }
Out << format_hex(apf.bitcastToAPInt().getZExtValue(), 0, /*Upper=*/true);
return;
}
diff --git a/llvm/lib/Support/APFloat.cpp b/llvm/lib/Support/APFloat.cpp
index 58e49b5384cd5..c79fc8a63de19 100644
--- a/llvm/lib/Support/APFloat.cpp
+++ b/llvm/lib/Support/APFloat.cpp
@@ -2243,26 +2243,15 @@ 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);
+ // Convert of sNaN creates qNaN and raises an exception (invalid op).
+ // This also guarantees that a sNaN does not become Inf on a truncation
+ // that loses all payload bits.
+ if (isSignaling()) {
+ makeQuiet();
+ fs = opInvalidOp;
+ } else {
+ fs = opOK;
}
-
- // 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
- // an invalid operation signal at runtime, but nobody does that.
- fs = opOK;
} else {
*losesInfo = false;
fs = opOK;
diff --git a/llvm/test/Transforms/InstSimplify/ConstProp/cast.ll b/llvm/test/Transforms/InstSimplify/ConstProp/cast.ll
index 41765be1f2c89..adf5e4b68a1b2 100644
--- a/llvm/test/Transforms/InstSimplify/ConstProp/cast.ll
+++ b/llvm/test/Transforms/InstSimplify/ConstProp/cast.ll
@@ -40,24 +40,24 @@ define float @overflow_sitofp() {
}
; https://llvm.org/PR43907 - make sure that NaN doesn't morph into Inf.
-; SNaN remains SNaN.
+; SNaN becomes QNaN.
define float @nan_f64_trunc() {
; CHECK-LABEL: @nan_f64_trunc(
-; CHECK-NEXT: ret float 0x7FF4000000000000
+; CHECK-NEXT: ret float 0x7FF8000000000000
;
%f = fptrunc double 0x7FF0000000000001 to float
ret float %f
}
; Verify again with a vector and
diff erent destination type.
-; SNaN remains SNaN (first two elements).
+; SNaN becomes 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>
+; CHECK-NEXT: ret <3 x half> <half 0xH7E00, half 0xH7E00, half 0xH7E00>
;
%f = fptrunc <3 x double> <double 0x7FF0020000000000, double 0x7FF003FFFFFFFFFF, double 0x7FF8000000000001> to <3 x half>
ret <3 x half> %f
diff --git a/llvm/test/Transforms/PhaseOrdering/X86/nancvt.ll b/llvm/test/Transforms/PhaseOrdering/X86/nancvt.ll
index c87390c268c91..1ea183b46a4bf 100644
--- a/llvm/test/Transforms/PhaseOrdering/X86/nancvt.ll
+++ b/llvm/test/Transforms/PhaseOrdering/X86/nancvt.ll
@@ -18,6 +18,9 @@ target triple = "i686-apple-darwin8"
@var = external global i32
+; SNAN becomes QNAN on fptrunc:
+; 2147228864 = 0x7ffc1cc0 : QNAN
+
define i32 @main() {
; CHECK-LABEL: @main(
; CHECK-NEXT: entry:
@@ -30,15 +33,15 @@ define i32 @main() {
; CHECK-NEXT: store volatile i32 2147228864, i32* @var, align 4
; CHECK-NEXT: store volatile i32 2147228864, i32* @var, align 4
; CHECK-NEXT: store volatile i32 2147228864, i32* @var, align 4
-; CHECK-NEXT: store volatile i32 2146502828, i32* @var, align 4
+; CHECK-NEXT: store volatile i32 2147027116, i32* @var, align 4
; CHECK-NEXT: store volatile i32 -1610612736, i32* @var, align 4
-; CHECK-NEXT: store volatile i32 2146502828, i32* @var, align 4
+; CHECK-NEXT: store volatile i32 2147027116, i32* @var, align 4
; CHECK-NEXT: store volatile i32 -2147483648, i32* @var, align 4
-; CHECK-NEXT: store volatile i32 2146502828, i32* @var, align 4
+; CHECK-NEXT: store volatile i32 2147027116, i32* @var, align 4
; CHECK-NEXT: store volatile i32 -1073741824, i32* @var, align 4
-; CHECK-NEXT: store volatile i32 2143034560, i32* @var, align 4
-; CHECK-NEXT: store volatile i32 2143034560, i32* @var, align 4
-; CHECK-NEXT: store volatile i32 2143034560, i32* @var, align 4
+; CHECK-NEXT: store volatile i32 2147228864, i32* @var, align 4
+; CHECK-NEXT: store volatile i32 2147228864, i32* @var, align 4
+; CHECK-NEXT: store volatile i32 2147228864, i32* @var, align 4
; CHECK-NEXT: ret i32 undef
;
entry:
diff --git a/llvm/unittests/ADT/APFloatTest.cpp b/llvm/unittests/ADT/APFloatTest.cpp
index 475ad83e2d9d1..2088df0b4d3f2 100644
--- a/llvm/unittests/ADT/APFloatTest.cpp
+++ b/llvm/unittests/ADT/APFloatTest.cpp
@@ -1816,11 +1816,12 @@ TEST(APFloatTest, convert) {
EXPECT_FALSE(losesInfo);
test = APFloat::getSNaN(APFloat::IEEEsingle());
- APFloat X87SNaN = APFloat::getSNaN(APFloat::x87DoubleExtended());
APFloat::opStatus status = test.convert(APFloat::x87DoubleExtended(), APFloat::rmNearestTiesToEven, &losesInfo);
- EXPECT_TRUE(test.bitwiseIsEqual(X87SNaN));
+ // Conversion quiets the SNAN, so now 2 bits of the 64-bit significand should be set.
+ APInt topTwoBits(64, 0x6000000000000000);
+ EXPECT_TRUE(test.bitwiseIsEqual(APFloat::getQNaN(APFloat::x87DoubleExtended(), false, &topTwoBits)));
EXPECT_FALSE(losesInfo);
- EXPECT_EQ(status, APFloat::opOK);
+ EXPECT_EQ(status, APFloat::opInvalidOp);
test = APFloat::getQNaN(APFloat::IEEEsingle());
APFloat X87QNaN = APFloat::getQNaN(APFloat::x87DoubleExtended());
@@ -1832,6 +1833,7 @@ TEST(APFloatTest, convert) {
test = APFloat::getSNaN(APFloat::x87DoubleExtended());
test.convert(APFloat::x87DoubleExtended(), APFloat::rmNearestTiesToEven,
&losesInfo);
+ APFloat X87SNaN = APFloat::getSNaN(APFloat::x87DoubleExtended());
EXPECT_TRUE(test.bitwiseIsEqual(X87SNaN));
EXPECT_FALSE(losesInfo);
@@ -1841,13 +1843,13 @@ TEST(APFloatTest, convert) {
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.
+ // The payload is lost in truncation, but we retain NaN by setting the quiet bit.
APInt payload(52, 1);
test = APFloat::getSNaN(APFloat::IEEEdouble(), false, &payload);
status = test.convert(APFloat::IEEEsingle(), APFloat::rmNearestTiesToEven, &losesInfo);
- EXPECT_EQ(0x7fa00000, test.bitcastToAPInt());
+ EXPECT_EQ(0x7fc00000, test.bitcastToAPInt());
EXPECT_TRUE(losesInfo);
- EXPECT_EQ(status, APFloat::opOK);
+ EXPECT_EQ(status, APFloat::opInvalidOp);
// The payload is lost in truncation. QNaN remains QNaN.
test = APFloat::getQNaN(APFloat::IEEEdouble(), false, &payload);
More information about the cfe-commits
mailing list