-
Notifications
You must be signed in to change notification settings - Fork 15k
[ADT] Fix a bug in DoubleAPFloat::frexp #161625
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
[ADT] Fix a bug in DoubleAPFloat::frexp #161625
Conversation
Without this patch, we call APFloat::makeQuiet() in frexp like so: Quiet.getFirst().makeQuiet(); The problem is that makeQuiet returns a new value instead of modifying "*this" in place, so we end up discarding the newly returned value. This patch fixes the problem by assigning the result back to Quiet.getFirst(). We should put [[nodiscard]] on APFloat::makeQuiet, but I'll do that in another patch.
|
@llvm/pr-subscribers-llvm-adt @llvm/pr-subscribers-llvm-support Author: Kazu Hirata (kazutakahirata) ChangesWithout this patch, we call APFloat::makeQuiet() in frexp like so: Quiet.getFirst().makeQuiet(); The problem is that makeQuiet returns a new value instead of modifying This patch fixes the problem by assigning the result back to We should put [[nodiscard]] on APFloat::makeQuiet, but I'll do that in Full diff: https://github.com/llvm/llvm-project/pull/161625.diff 2 Files Affected:
diff --git a/llvm/lib/Support/APFloat.cpp b/llvm/lib/Support/APFloat.cpp
index d14abb4bd05b5..8623c06597f5c 100644
--- a/llvm/lib/Support/APFloat.cpp
+++ b/llvm/lib/Support/APFloat.cpp
@@ -5857,7 +5857,7 @@ DoubleAPFloat frexp(const DoubleAPFloat &Arg, int &Exp,
// practice.
if (Exp == APFloat::IEK_NaN) {
DoubleAPFloat Quiet{Arg};
- Quiet.getFirst().makeQuiet();
+ Quiet.getFirst() = Quiet.getFirst().makeQuiet();
return Quiet;
}
diff --git a/llvm/unittests/ADT/APFloatTest.cpp b/llvm/unittests/ADT/APFloatTest.cpp
index 141282ea254b4..30f0a8e5089ef 100644
--- a/llvm/unittests/ADT/APFloatTest.cpp
+++ b/llvm/unittests/ADT/APFloatTest.cpp
@@ -10176,4 +10176,11 @@ TEST(APFloatTest, hasSignBitInMSB) {
EXPECT_FALSE(APFloat::hasSignBitInMSB(APFloat::Float8E8M0FNU()));
}
+TEST(APFloatTest, FrexpQuietSNaN) {
+ APFloat SNaN = APFloat::getSNaN(APFloat::PPCDoubleDouble());
+ int Exp;
+ APFloat Result = frexp(SNaN, Exp, APFloat::rmNearestTiesToEven);
+ EXPECT_FALSE(Result.isSignaling());
+}
+
} // namespace
|
Without this patch, we call APFloat::makeQuiet() in frexp like so: Quiet.getFirst().makeQuiet(); The problem is that makeQuiet returns a new value instead of modifying "*this" in place, so we end up discarding the newly returned value. This patch fixes the problem by assigning the result back to Quiet.getFirst(). We should put [[nodiscard]] on APFloat::makeQuiet, but I'll do that in another patch.
FWIW, this [[nodiscard]] led to the discovery of #161625.
FWIW, this [[nodiscard]] led to the discovery of llvm#161625.
FWIW, this [[nodiscard]] led to the discovery of llvm#161625.
Without this patch, we call APFloat::makeQuiet() in frexp like so:
Quiet.getFirst().makeQuiet();
The problem is that makeQuiet returns a new value instead of modifying
"*this" in place, so we end up discarding the newly returned value.
This patch fixes the problem by assigning the result back to
Quiet.getFirst().
We should put [[nodiscard]] on APFloat::makeQuiet, but I'll do that in
another patch.