[APFloat] Fix literals with long significands. #102051
Conversation
Replace fixed-size arrays in powerOf5 with SmallVector to support significands longer than about 10000 digits. The new limit should be around UINT_MAX/815, practically limited by processing time. Fixes llvm#28406
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-llvm-adt @llvm/pr-subscribers-llvm-support Author: None (keinflue) ChangesThis is my first contribution attempt to LLVM. Please let me know of any issues. The function
This PR replaces the arrays with The new definitive limit for the length of the sum of significand and exponent is However, processing time will likely be the practical limitation. Open questions:
I compiled and verified Fixes #28406 Full diff: https://github.com/llvm/llvm-project/pull/102051.diff 1 Files Affected:
diff --git a/llvm/lib/Support/APFloat.cpp b/llvm/lib/Support/APFloat.cpp
index 7f68c5ab9b7cf..0f91953cca301 100644
--- a/llvm/lib/Support/APFloat.cpp
+++ b/llvm/lib/Support/APFloat.cpp
@@ -18,6 +18,7 @@
#include "llvm/ADT/FoldingSet.h"
#include "llvm/ADT/Hashing.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Config/llvm-config.h"
@@ -27,6 +28,7 @@
#include "llvm/Support/raw_ostream.h"
#include <cstring>
#include <limits.h>
+#include <limits>
#define APFLOAT_DISPATCH_ON_SEMANTICS(METHOD_CALL) \
do { \
@@ -308,24 +310,6 @@ constexpr RoundingMode APFloatBase::rmTowardNegative;
constexpr RoundingMode APFloatBase::rmTowardZero;
constexpr RoundingMode APFloatBase::rmNearestTiesToAway;
-/* A tight upper bound on number of parts required to hold the value
- pow(5, power) is
-
- power * 815 / (351 * integerPartWidth) + 1
-
- However, whilst the result may require only this many parts,
- because we are multiplying two values to get it, the
- multiplication may require an extra part with the excess part
- being zero (consider the trivial case of 1 * 1, tcFullMultiply
- requires two parts to hold the single-part result). So we add an
- extra one to guarantee enough space whilst multiplying. */
-const unsigned int maxExponent = 16383;
-const unsigned int maxPrecision = 113;
-const unsigned int maxPowerOfFiveExponent = maxExponent + maxPrecision - 1;
-const unsigned int maxPowerOfFiveParts =
- 2 +
- ((maxPowerOfFiveExponent * 815) / (351 * APFloatBase::integerPartWidth));
-
unsigned int APFloatBase::semanticsPrecision(const fltSemantics &semantics) {
return semantics.precision;
}
@@ -763,25 +747,44 @@ ulpsFromBoundary(const APFloatBase::integerPart *parts, unsigned int bits,
/* Place pow(5, power) in DST, and return the number of parts used.
DST must be at least one part larger than size of the answer. */
-static unsigned int
-powerOf5(APFloatBase::integerPart *dst, unsigned int power) {
+static SmallVector<APFloatBase::integerPart, 0> powerOf5(unsigned int power) {
static const APFloatBase::integerPart firstEightPowers[] = { 1, 5, 25, 125, 625, 3125, 15625, 78125 };
- APFloatBase::integerPart pow5s[maxPowerOfFiveParts * 2 + 5];
+
+ // Make sure that the following expression does not overflow.
+ assert(power <= UINT_MAX / 815);
+
+ // A tight upper bound on number of parts required to hold the value
+ // pow(5, power) is
+ //
+ // power * 815 / (351 * integerPartWidth) + 1
+ //
+ // However, whilst the result may require only this many parts,
+ // because we are multiplying two values to get it, the
+ // multiplication may require an extra part with the excess part
+ // being zero (consider the trivial case of 1 * 1, tcFullMultiply
+ // requires two parts to hold the single-part result). So we add an
+ // extra one to guarantee enough space whilst multiplying.
+ const unsigned int maxPowerOfFiveParts =
+ 2 + ((power * 815) / (351 * APFloatBase::integerPartWidth));
+
+ SmallVector<APFloatBase::integerPart, 0> pow5s(maxPowerOfFiveParts * 2 + 5);
pow5s[0] = 78125 * 5;
unsigned int partsCount = 1;
- APFloatBase::integerPart scratch[maxPowerOfFiveParts], *p1, *p2, *pow5;
- unsigned int result;
- assert(power <= maxExponent);
+ SmallVector<APFloatBase::integerPart, 0> scratch(maxPowerOfFiveParts);
+ APFloatBase::integerPart *p1, *p2, *pow5;
+ unsigned int resultSize;
+
+ SmallVector<APFloatBase::integerPart, 0> dst(maxPowerOfFiveParts);
- p1 = dst;
- p2 = scratch;
+ p1 = dst.data();
+ p2 = scratch.data();
*p1 = firstEightPowers[power & 7];
power >>= 3;
- result = 1;
- pow5 = pow5s;
+ resultSize = 1;
+ pow5 = pow5s.data();
for (unsigned int n = 0; power; power >>= 1, n++) {
/* Calculate pow(5,pow(2,n+3)) if we haven't yet. */
@@ -796,10 +799,10 @@ powerOf5(APFloatBase::integerPart *dst, unsigned int power) {
if (power & 1) {
APFloatBase::integerPart *tmp;
- APInt::tcFullMultiply(p2, p1, pow5, result, partsCount);
- result += partsCount;
- if (p2[result - 1] == 0)
- result--;
+ APInt::tcFullMultiply(p2, p1, pow5, resultSize, partsCount);
+ resultSize += partsCount;
+ if (p2[resultSize - 1] == 0)
+ resultSize--;
/* Now result is in p1 with partsCount parts and p2 is scratch
space. */
@@ -811,10 +814,14 @@ powerOf5(APFloatBase::integerPart *dst, unsigned int power) {
pow5 += partsCount;
}
- if (p1 != dst)
- APInt::tcAssign(dst, p1, result);
+ if (p1 != dst.data())
+ APInt::tcAssign(dst.data(), p1, resultSize);
- return result;
+ // Truncate to the actually used elements from the initial worst-case sizing
+ // during initialization.
+ dst.truncate(resultSize);
+
+ return dst;
}
/* Zero at the end to avoid modular arithmetic when adding one; used
@@ -2950,9 +2957,9 @@ IEEEFloat::opStatus
IEEEFloat::roundSignificandWithExponent(const integerPart *decSigParts,
unsigned sigPartCount, int exp,
roundingMode rounding_mode) {
- unsigned int parts, pow5PartCount;
- fltSemantics calcSemantics = { 32767, -32767, 0, 0 };
- integerPart pow5Parts[maxPowerOfFiveParts];
+ unsigned int parts;
+ fltSemantics calcSemantics = {std::numeric_limits<ExponentType>::max(),
+ std::numeric_limits<ExponentType>::min(), 0, 0};
bool isNearest;
isNearest = (rounding_mode == rmNearestTiesToEven ||
@@ -2960,8 +2967,11 @@ IEEEFloat::roundSignificandWithExponent(const integerPart *decSigParts,
parts = partCountForBits(semantics->precision + 11);
+ // Make sure that abs(exp) is representable.
+ assert(exp > INT_MIN);
+
/* Calculate pow(5, abs(exp)). */
- pow5PartCount = powerOf5(pow5Parts, exp >= 0 ? exp: -exp);
+ SmallVector<integerPart, 0> pow5Parts = powerOf5(exp >= 0 ? exp : -exp);
for (;; parts *= 2) {
opStatus sigStatus, powStatus;
@@ -2977,8 +2987,8 @@ IEEEFloat::roundSignificandWithExponent(const integerPart *decSigParts,
sigStatus = decSig.convertFromUnsignedParts(decSigParts, sigPartCount,
rmNearestTiesToEven);
- powStatus = pow5.convertFromUnsignedParts(pow5Parts, pow5PartCount,
- rmNearestTiesToEven);
+ powStatus = pow5.convertFromUnsignedParts(
+ pow5Parts.data(), pow5Parts.size(), rmNearestTiesToEven);
/* Add exp, as 10^n = 5^n * 2^n. */
decSig.exponent += exp;
|
|
Ping |
|
Can you add a test to unittests/ADT/APFloatTest.cpp for a case that would have failed previously? |
|
ping |
| const unsigned int maxPowerOfFiveParts = | ||
| 2 + ((power * 815) / (351 * APFloatBase::integerPartWidth)); | ||
|
|
||
| SmallVector<APFloatBase::integerPart, 0> pow5s(maxPowerOfFiveParts * 2 + 5); |
There was a problem hiding this comment.
, 0> is a heap allocation, I don't think that's what you want?
Either , 1> or just not specifying it
There was a problem hiding this comment.
The actual minimum size required (see lines above) is 9 and I think SmallVector's default is smaller. It increases with both the length of the significand of the floating point literal as well as its exponent. I used 0 because I have no idea what a good estimation for typical floating point literals is. The old code used a fixed size array of roughly length 1200. I could of course use that, but felt that it was a bit much waste of stack space for the typical use case. All in all the arrays took roughly 14kB stack.
| bool isNearest; | ||
|
|
||
| isNearest = (rounding_mode == rmNearestTiesToEven || | ||
| rounding_mode == rmNearestTiesToAway); | ||
|
|
||
| parts = partCountForBits(semantics->precision + 11); | ||
|
|
||
| // Make sure that abs(exp) is representable. | ||
| assert(exp > INT_MIN); |
There was a problem hiding this comment.
Why's this an assert()? Shouldn't it fail in some interesting way?
There was a problem hiding this comment.
Why's this an
assert()? Shouldn't it fail in some interesting way?
With the limited range of supported exponents this could only happen at this point (if at all) if the significand is almost INT_MAX digits long. That's an edge case that I haven't looked at in more detail. I expect that this will already fail earlier. In any case, it is not really relevant for the functionality in this PR. I simply added it when I saw the std::abs-type construct following it as a default precaution to catch the usual UB edge case. I can remove it again as well.
In principle the function should report this as failure and on the Clang side a diagnostic about implementation limits should be produced - for the new assert in power5 as well - but I wasn't sure whether that would be in scope for this bug fix, since there was no error reporting for the implementation limit beforehand either. I simply increase the implied limit without fixing the lack of error reporting to keep the scope of this first contribution small. Let me know if you'd like to see that fixed as par of this PR as well.
Also prefer std::numeric_limits
Will do. I kept using the macros in style with the file. If it wasn't for that, I would have always preferred std::numeric_limits.
| const unsigned int maxPowerOfFiveParts = | ||
| 2 + ((power * 815) / (351 * APFloatBase::integerPartWidth)); | ||
|
|
||
| SmallVector<APFloatBase::integerPart, 0> pow5s(maxPowerOfFiveParts * 2 + 5); |
There was a problem hiding this comment.
The actual minimum size required (see lines above) is 9 and I think SmallVector's default is smaller. It increases with both the length of the significand of the floating point literal as well as its exponent. I used 0 because I have no idea what a good estimation for typical floating point literals is. The old code used a fixed size array of roughly length 1200. I could of course use that, but felt that it was a bit much waste of stack space for the typical use case. All in all the arrays took roughly 14kB stack.
| bool isNearest; | ||
|
|
||
| isNearest = (rounding_mode == rmNearestTiesToEven || | ||
| rounding_mode == rmNearestTiesToAway); | ||
|
|
||
| parts = partCountForBits(semantics->precision + 11); | ||
|
|
||
| // Make sure that abs(exp) is representable. | ||
| assert(exp > INT_MIN); |
There was a problem hiding this comment.
Why's this an
assert()? Shouldn't it fail in some interesting way?
With the limited range of supported exponents this could only happen at this point (if at all) if the significand is almost INT_MAX digits long. That's an edge case that I haven't looked at in more detail. I expect that this will already fail earlier. In any case, it is not really relevant for the functionality in this PR. I simply added it when I saw the std::abs-type construct following it as a default precaution to catch the usual UB edge case. I can remove it again as well.
In principle the function should report this as failure and on the Clang side a diagnostic about implementation limits should be produced - for the new assert in power5 as well - but I wasn't sure whether that would be in scope for this bug fix, since there was no error reporting for the implementation limit beforehand either. I simply increase the implied limit without fixing the lack of error reporting to keep the scope of this first contribution small. Let me know if you'd like to see that fixed as par of this PR as well.
Also prefer std::numeric_limits
Will do. I kept using the macros in style with the file. If it wasn't for that, I would have always preferred std::numeric_limits.
This is my first contribution attempt to LLVM. Please let me know of any issues.
The function
powerOf5is called (only) byconvertFromDecimalStringto calculate 5^m where m is sum of the length of the significand and the magnitude of the (normalized) exponent.powerOf5previously used fixed-sized arrays to store its (intermediate) results. Their size was chosen large enough to hold the sum of a maximum exponent and a maximum precision. However, this didn't account for literals with long overspecifying significands.This PR replaces the arrays with
SmallVectors to also support these cases.The new definitive limit for the length of the sum of significand and exponent is
UINT_MAX/815(~5M) due to an overflow in the length calculation for theSmallVectors.However, processing time will likely be the practical limitation.
Open questions:
SmallVector's inline size is chosen as0, because the vectors will usually not be that small. Alternatives: Use the previous fixed-size array length or some value corresponding to short common literals.calcSemanticsare replaced with the largest possible range. I am not sure whether this is allowed.I compiled and verified
ninja check-llvm,ninja check-llvm-unitandninja check-clangon Linux x86-64.Fixes #28406