[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:1483
@@ +1482,3 @@
+    // If the number is of unknown sign, the highest possibly set bit is the
+    // higher of the lowest known zero and the highest not known zero.
+
----------------
eli.friedman wrote:
> Leftover comment?
I got rid of that in my local copy but I didn't update my diff. Thanks for catching that.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1507
@@ +1506,3 @@
+    int BitRange = MostSignificantPossiblySetBit -
+                LeastSignificantPossiblySetBit + 1;
+
----------------
eli.friedman wrote:
> Doesn't the + 1 here cancel out the -1 in the setting of MostSignificantPossiblySetBit?  That would make the math a bit more straightforward.
The reasoning behind this is that I wanted the values for both least and most significant possibly set bit to be zero indexed; I thought it might be confusing if one was and the other wasn't.

================
Comment at: test/Transforms/InstCombine/sitofp.ll:272
@@ +271,2 @@
+ ret i32 %D
+}
----------------
eli.friedman wrote:
> Did you find any interesting new cases worth having a separate regression test?
Yes, I will add some tests with unknown sign.


https://reviews.llvm.org/D19178