[PATCH] D19178: Broaden FoldItoFPtoI to try and establish whether the integer value fits into the float type

[Carlos Liam via llvm-commits]

aarzee added inline comments.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1426
@@ -1424,1 +1425,3 @@
+// least significant set bits of abs(X) fits into the mantissa, and that the
+// number of trailing zeros fits into the exponent.
 Instruction *InstCombiner::FoldItoFPtoI(Instruction &FI) {
----------------
scanon wrote:
> A few things:
> 
> 1. This:
> 
> > and that the number of trailing zeros fits into the exponent
> 
> is really confused.  That's not how it [floating-point encoding] works at all.  A correct algorithm for determining if an integer n can be exactly represented is (pseudocode):
> ```
> m = absoluteValue(n)
> i = mostSignificantNonzeroBit(m)
> j = leastSignificantNonzeroBit(m)
> if (i - j < numberOfSignificandBits) {
>   // significand is representable, check exponent
>   if (i <= greatestFiniteExponent) {
>     // number can be exactly represented
>   }
> }
> // number cannot be exactly represented
> ```
> In particular, if we look at your example, `(2 ** 24 - 1) << 10`, this number absolutely is exactly representable in single-precision.  We have:
> ```
> m = 2**24-1 << 10
> i = 33
> j = 10
> i-j = 23 < numberOfSignificandBits = 24
> i = 33 <= greatestFiniteExponent = 127.
> ```
> 
> 2. Please don't use the term "mantissa".  It is frequently used, even in the LLVM sources, but it is subtly wrong (https://en.wikipedia.org/wiki/Significand#Use_of_.22mantissa.22).  The correct term is "significand"; any new written code should use that instead.
Shouldn't it be `j <= greatestFiniteExponent`? (I'm clearly not an expert here, so I just wanted to check).


http://reviews.llvm.org/D19178