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

[Steve Canon via llvm-commits]

scanon 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) {
----------------
aarzee wrote:
> 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).
No, it should be `i`.  Normal binary floating-point numbers have the form +/- significand * 2^exponent, where significand is a number in [1,2).  Thus, the MSB a representable integer is always 2^exponent.


http://reviews.llvm.org/D19178