[Support][APInt] Fix sign extension, exponent and mantissa in APInt::roundToDouble#192451
[Support][APInt] Fix sign extension, exponent and mantissa in APInt::roundToDouble#192451
Conversation
|
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-llvm-adt Author: Andi Drebes (andidr) ChangesConversion of an The assertion is triggered when attempting to convert a multi-word
The issue is caused by passing the bit width as the argument to Incorrect values for the mantissa or exponent are calculated for any multi-word, unsigned value, e.g.,
This is due to This patch solves the issues by simplifying the treatment of single-word integers and by adjusting the exponent and mantissa for multi-word integers. Tests are added for the edge cases and some regular cases. Full diff: https://github.com/llvm/llvm-project/pull/192451.diff 2 Files Affected:
diff --git a/llvm/lib/Support/APInt.cpp b/llvm/lib/Support/APInt.cpp
index 6aa5fc615a302..1d1765a9e178b 100644
--- a/llvm/lib/Support/APInt.cpp
+++ b/llvm/lib/Support/APInt.cpp
@@ -900,12 +900,9 @@ APInt llvm::APIntOps::RoundDoubleToAPInt(double Double, unsigned width) {
double APInt::roundToDouble(bool isSigned) const {
// Handle the simple case where the value is contained in one uint64_t.
// It is wrong to optimize getWord(0) to VAL; there might be more than one word.
- if (isSingleWord() || getActiveBits() <= APINT_BITS_PER_WORD) {
- if (isSigned) {
- int64_t sext = SignExtend64(getWord(0), BitWidth);
- return double(sext);
- }
- return double(getWord(0));
+ if (isSingleWord()) {
+ return isSigned ? double(bit_cast<int64_t>(getWord(0)))
+ : double(getWord(0));
}
// Determine if the value is negative.
@@ -917,10 +914,15 @@ double APInt::roundToDouble(bool isSigned) const {
// Figure out how many bits we're using.
unsigned n = Tmp.getActiveBits();
- // The exponent (without bias normalization) is just the number of bits
- // we are using. Note that the sign bit is gone since we constructed the
- // absolute value.
- uint64_t exp = n;
+ // Early exit for 0 to avoid negative indexes
+ if (n == 0)
+ return 0.0;
+
+ // The exponent (without bias normalization) is just the number of
+ // bits we are using (minus 1 to account for the fact that the
+ // exponent is on 2). Note that the sign bit is gone since we
+ // constructed the absolute value.
+ uint64_t exp = n-1;
// Return infinity for exponent overflow
if (exp > 1023) {
@@ -947,6 +949,7 @@ double APInt::roundToDouble(bool isSigned) const {
}
// The leading bit of mantissa is implicit, so get rid of it.
+ mantissa &= ~(1ULL << std::min(n-1, 51U));
uint64_t sign = isNeg ? (1ULL << (APINT_BITS_PER_WORD - 1)) : 0;
uint64_t I = sign | (exp << 52) | mantissa;
return bit_cast<double>(I);
diff --git a/llvm/unittests/ADT/APIntTest.cpp b/llvm/unittests/ADT/APIntTest.cpp
index ee4c59de34fc4..d5fe5b30b2fd7 100644
--- a/llvm/unittests/ADT/APIntTest.cpp
+++ b/llvm/unittests/ADT/APIntTest.cpp
@@ -3980,4 +3980,34 @@ TEST(APIntTest, clmulh) {
.getSExtValue(),
21845);
}
+
+TEST(APIntTest, roundToDouble) {
+ // Single-word, positive
+ EXPECT_EQ(APInt(64, 0, false).roundToDouble(false), 0.0);
+ EXPECT_EQ(APInt(64, 1, false).roundToDouble(false), 1.0);
+ EXPECT_EQ(APInt(64, 2, false).roundToDouble(false), 2.0);
+ EXPECT_EQ(APInt(64, 1ULL << 63, false).roundToDouble(false), 9223372036854775808.0);
+
+ // Single-word, negative
+ EXPECT_EQ(APInt(64, 0, true).roundToDouble(true), 0.0);
+ EXPECT_EQ(APInt(64, -1, true).roundToDouble(true), -1.0);
+ EXPECT_EQ(APInt(64, -2, true).roundToDouble(true), -2.0);
+ EXPECT_EQ(APInt(64, 1ULL << 63, true).roundToDouble(true), -9223372036854775808.0);
+
+ // Multi-word, positive, active bits in first word
+ EXPECT_EQ(APInt(65, 0, false).roundToDouble(false), 0.0);
+ EXPECT_EQ(APInt(65, 1, false).roundToDouble(false), 1.0);
+ EXPECT_EQ(APInt(65, 2, false).roundToDouble(false), 2.0);
+ EXPECT_EQ(APInt(65, 1ULL << 63, false).roundToDouble(true), 9223372036854775808.0);
+
+ // Multi-word, positive, active bits outside first word
+ EXPECT_EQ(APInt(65, "18446744073709551616" /* 2^64 */, 10).roundToDouble(false), 18446744073709551616.0);
+
+ // Multi-word, negative
+ EXPECT_EQ(APInt(65, 0, true).roundToDouble(true), 0.0);
+
+ EXPECT_EQ(APInt(65, -1, true).roundToDouble(true), -1.0);
+ EXPECT_EQ(APInt(65, -2, true).roundToDouble(true), -2.0);
+ EXPECT_EQ(APInt(65, "18446744073709551616" /* 2^64 */, 10).roundToDouble(true), -18446744073709551616.0);
+}
} // end anonymous namespace
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
andidr
force-pushed
the
apint-fixes
branch
2 times, most recently
from
ac107f6 to
0b4c3f8
Compare
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
|
I think it would be better to rewrite this function from scratch. It has several issues, the main one is that exact integer should round to the nearest representable double (with ties to even) but not it just chops extra low bits (trunc). Even better to add a few rounding options as an optional enum argument. |
| } | ||
| return double(getWord(0)); | ||
| if (isSingleWord()) { | ||
| return isSigned ? double(bit_cast<int64_t>(getWord(0))) |
APFloat::convertFromAPInt already exists. |
I see, but this will require Btw double rounded conversion |
|
We could implement roundToDouble as a wrapper around convertFromAPInt, I guess. |
…roundToDouble Conversion of an `APInt` to double via `APInt::roundToDouble` misses several edge cases that result in crashes due to an assertion or in erroneous values due to incorrect values for the exponent and mantissa of the generated double. The assertion is triggered when attempting to convert a multi-word `APInt` without active bits or with active bits only in the first word, e.g., `APInt(65, 0, true).roundToDouble(true)` or `APInt(65, 1, true).roundToDouble(true)` The issue is caused by passing the bit width as the argument to `SignExtend64`, exceeding the maximum expected bit width of 64. Incorrect values for the mantissa or exponent are calculated for any multi-word, unsigned value, e.g., `APInt(65, "18446744073709551616" /* 2^64 */, 10).roundToDouble(false)` This is due to `APInt::roundToDouble` expecting the exponent to correspond to the highest of the fractional part of the double rather than to the highest active bit and due to the missing code clearing the highest bit after generation of the mantissa. This patch solves the issues by by delegating the conversion to `APFloat::convertFromAPInt` with double semantics and rounding towards zero. Tests are added for the edge cases and some regular cases.
|
Thanks for the comments. I pushed a new version that delegates the conversion work to |
| } | ||
| APFloat f(APFloat::IEEEdouble()); | ||
| f.convertFromAPInt(*this, isSigned, | ||
| llvm::APFloatBase::roundingMode::TowardZero); |
There was a problem hiding this comment.
I still think it should be round to nearest integer "ties to even" instead "towards zero". Floating point semantics matching IEC 60559 (IEEE 754) are defined in Annex F of the standard, which technically is optional and IB, but in practice many compilers and libraries for targets and runtimes (like CUDA, WebAssembly) use default ties to even during integer -> float point conversion. So I’m genuinely curious as to why this particular rounding method was chosen in the first place
There was a problem hiding this comment.
This method was written almost 20 years ago, and I think it’s worth figuring out what it’s actually used for:
d707d63
There was a problem hiding this comment.
Neither the commit message, nor the comments give a hint about why that rounding mode was chosen. This looks more like a practical decision for the implementation rather than a deliberate choice.
Unit tests seem to pass with roundingMode::NearestTiesToEven, but it feels odd to change the semantics.
There was a problem hiding this comment.
How did you run into issues with roundToDouble? Even a quick glance shows that this method it seems used only in the ExecutionEngine. MCJIT in LLVM not used widely due to it very slow. For example Julia language uses ORCv2 JIT, which doesn't utilize ExecutionEngine. So the question is should we even fix this? Maybe it would be easier to mark it as deprecated or even remove it (but this breaking changes)?
There was a problem hiding this comment.
I stumbled across this while working on a non-public project that needs to convert APInts to doubles in a compilation pass.
There was a problem hiding this comment.
It seems that lldb uses this indirectly, at least via the tests from lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py (e.g., this run of a previous version of the PR failed due to APInt::roundToDouble() producing wrong results).
So what do you think of keeping the current rounding semantics and marking the function as deprecated?
There was a problem hiding this comment.
We can't mark functions deprecated when they still have in-tree uses, I think; it'll trigger errors on the buidbots. Or at least, each use would need to explicitly suppress the deprecation warning.
If you want to go through and push patches to clean up the users, I don't think there are actually very many.
There was a problem hiding this comment.
(You could, instead of using the "deprecated" attribute, add a comment that says "please don't use this", but that's less helpful.)
5 participants
Conversion of an
APIntto double viaAPInt::roundToDoublemisses several edge cases that result in crashes due to an assertion or in erroneous values due to incorrect values for the exponent and mantissa of the generated double.The assertion is triggered when attempting to convert a multi-word
APIntwithout active bits or with active bits only in the first word, e.g.,APInt(65, 0, true).roundToDouble(true)orAPInt(65, 1, true).roundToDouble(true)The issue is caused by passing the bit width as the argument to
SignExtend64, exceeding the maximum expected bit width of 64.Incorrect values for the mantissa or exponent are calculated for any multi-word, unsigned value, e.g.,
APInt(65, "18446744073709551616" /* 2^64 */, 10).roundToDouble(false)This is due to
APInt::roundToDoubleexpecting the exponent to correspond to the highest of the fractional part of the double rather than to the highest active bit and due to the missing code clearing the highest bit after generation of the mantissa.This patch solves the issues by simplifying the treatment of single-word integers and by adjusting the exponent and mantissa for multi-word integers. Tests are added for the edge cases and some regular cases.