[llvm] [APFloat] Fix literals with long significands. (PR #102051)

via llvm-commits llvm-commits at lists.llvm.org
Tue Sep 3 14:22:23 PDT 2024


================
@@ -2950,18 +2956,21 @@ 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 ||
                rounding_mode == rmNearestTiesToAway);
 
   parts = partCountForBits(semantics->precision + 11);
 
+  // Make sure that abs(exp) is representable.
+  assert(exp > INT_MIN);
----------------
keinflue wrote:

> 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`.

https://github.com/llvm/llvm-project/pull/102051


More information about the llvm-commits mailing list