Skip to content

[Support][APInt] Fix sign extension, exponent and mantissa in APInt::roundToDouble #192451

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.

Sign up for GitHub

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

Open
andidr wants to merge 1 commit into
base: main
Choose a base branch
from
+50 −51
Open

[Support][APInt] Fix sign extension, exponent and mantissa in APInt::roundToDouble #192451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 11 additions & 51 deletions llvm/lib/Support/APInt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
//===----------------------------------------------------------------------===//

#include "llvm/ADT/APInt.h"
#include "llvm/ADT/APFloat.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/FoldingSet.h"
#include "llvm/ADT/Hashing.h"
Expand Down Expand Up @@ -890,66 +891,25 @@ APInt llvm::APIntOps::RoundDoubleToAPInt(double Double, unsigned width) {
return isNeg ? -Tmp : Tmp;
}

/// This function converts this APInt to a double.
/// Converts this APInt to a double with rounding towards zero.
///
/// The layout for double is as following (IEEE Standard 754):
/// --------------------------------------
/// | Sign Exponent Fraction Bias |
/// |-------------------------------------- |
/// | 1[63] 11[62-52] 52[51-00] 1023 |
/// --------------------------------------
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));
}

// Determine if the value is negative.
bool isNeg = isSigned ? (*this)[BitWidth-1] : false;

// Construct the absolute value if we're negative.
APInt Tmp(isNeg ? -(*this) : (*this));

// 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 zero-length values; APFloat::convertFromAPInt only
// applies to values with at least one word.
if (BitWidth == 0)
return 0.0;

// Return infinity for exponent overflow
if (exp > 1023) {
if (!isSigned || !isNeg)
return std::numeric_limits<double>::infinity();
else
return -std::numeric_limits<double>::infinity();
}
exp += 1023; // Increment for 1023 bias

// Number of bits in mantissa is 52. To obtain the mantissa value, we must
// extract the high 52 bits from the correct words in pVal.
uint64_t mantissa;
unsigned hiWord = whichWord(n-1);
if (hiWord == 0) {
mantissa = Tmp.U.pVal[0];
if (n > 52)
mantissa >>= n - 52; // shift down, we want the top 52 bits.
} else {
assert(hiWord > 0 && "huh?");
uint64_t hibits = Tmp.U.pVal[hiWord] << (52 - n % APINT_BITS_PER_WORD);
uint64_t lobits = Tmp.U.pVal[hiWord-1] >> (11 + n % APINT_BITS_PER_WORD);
mantissa = hibits | lobits;
}
APFloat f(APFloat::IEEEdouble());
f.convertFromAPInt(*this, isSigned,
llvm::APFloatBase::roundingMode::TowardZero);
Copy link
Copy Markdown
Contributor

@MaxGraey MaxGraey Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@MaxGraey MaxGraey Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method was written almost 20 years ago, and I think it’s worth figuring out what it’s actually used for:
d707d63

Copy link
Copy Markdown
Contributor Author

@andidr andidr Apr 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@MaxGraey MaxGraey Apr 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Copy Markdown
Contributor Author

@andidr andidr Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I stumbled across this while working on a non-public project that needs to convert APInts to doubles in a compilation pass.

Copy link
Copy Markdown
Contributor Author

@andidr andidr Apr 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator

@efriedma-quic efriedma-quic Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

@efriedma-quic efriedma-quic Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(You could, instead of using the "deprecated" attribute, add a comment that says "please don't use this", but that's less helpful.)


// The leading bit of mantissa is implicit, so get rid of it.
uint64_t sign = isNeg ? (1ULL << (APINT_BITS_PER_WORD - 1)) : 0;
uint64_t I = sign | (exp << 52) | mantissa;
return bit_cast<double>(I);
return f.convertToDouble();
}

// Truncate to new width.
Expand Down
39 changes: 39 additions & 0 deletions llvm/unittests/ADT/APIntTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3980,4 +3980,43 @@ TEST(APIntTest, clmulh) {
.getSExtValue(),
21845);
}

TEST(APIntTest, roundToDouble) {
EXPECT_EQ(APInt(0, 0, false).roundToDouble(false), 0.0);

// 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