[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:1423
@@ +1422,3 @@
+// least significant set bits of abs(X) fits into the mantissa, and that the
+// number of trailing zeros fits into the exponent.
+
----------------
reames wrote:
> The second part of this comment doesn't make any sense to me.  I can understand why an integer value of less than mantissa size (actual bits uses) could be converted lossly to and from a floating point type.  Why does the exponent come into this?  Are you trying to reason about integer values larger than the mantissa with precise representations in floating point?
> 
> If so, could I ask that you split this into two patches:
> 1) Handle just the mantissa case.
> 2) Extend it to the full complexity.  
> 
> Doing it that way would make it far more obvious what's going on.  I would personally be comfortable signing off on the former, but would not be on the later.  
> 
> Also, extracting out a helper function called something like "hasPreciseFPRepresentation" would make this code far more clear.  (If I'm actually understanding it correctly at all!)
Yes, that's what I'm doing.

The exponent comes into play because (for example) `(2 ** 24 - 1) << 10` has 23 significant bits, which fits into the mantissa, but has 9 leading zeroes, which is too large for the exponent (8 bits).

Could you be more specific about the splitting of the patch? I'm not sure what you mean by 'just the mantissa case' - if you mean the integer fits into the mantissa without any exponent, that's what FoldItoFPtoI already does, but based on bit-width of the integer type, not the integer value.


http://reviews.llvm.org/D19178