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

[Philip Reames via llvm-commits]

reames added a subscriber: reames.
reames requested changes to this revision.
reames added a reviewer: reames.
This revision now requires changes to proceed.

================
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.
+
----------------
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!)

================
Comment at: test/Transforms/InstCombine/sitofp.ll:191
@@ +190,3 @@
+; CHECK-NEXT: ret i32
+define i32 @test20(i32 %A) nounwind {
+ %B = and i32 %A, 16777215
----------------
FYI, you're going to need a lot more test cases to convince me this is correct.  Even for the split patch.  


http://reviews.llvm.org/D19178