[llvm] [APFloat] Fix literals with long significands. (PR #102051)
via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 5 13:05:44 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-support
Author: None (keinflue)
<details>
<summary>Changes</summary>
This is my first contribution attempt to LLVM. Please let me know of any issues.
The function `powerOf5` is called (only) by `convertFromDecimalString` to calculate 5^m where m is sum of the length of the significand and the magnitude of the (normalized) exponent.
`powerOf5` previously 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 `SmallVector`s 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 the `SmallVector`s.
However, processing time will likely be the practical limitation.
Open questions:
- `SmallVector`'s inline size is chosen as `0`, 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.
- The new limit mentioned above is asserted. Is error reporting necessary instead?
- The exponent bounds of `calcSemantics` are replaced with the largest possible range. I am not sure whether this is allowed.
- The test case is still missing, because I am not sure whether the long literal from the linked issue should be embedded in a unit test or whether another kind of test would be preferred.
I compiled and verified `ninja check-llvm`, `ninja check-llvm-unit` and `ninja check-clang` on Linux x86-64.
Fixes #<!-- -->28406
---
Full diff: https://github.com/llvm/llvm-project/pull/102051.diff
1 Files Affected:
- (modified) llvm/lib/Support/APFloat.cpp (+51-41)
``````````diff
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;
``````````
</details>
https://github.com/llvm/llvm-project/pull/102051
More information about the llvm-commits
mailing list