[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:1507
@@ +1506,3 @@
+    // then the conversion is undefined behavior.
+
+    Safe = BitRange <= SignificandWidth;
----------------
eli.friedman wrote:
> aarzee wrote:
> > eli.friedman wrote:
> > > aarzee wrote:
> > > > eli.friedman wrote:
> > > > > Are you sure subtracting one actually makes the most significant bit zero-indexed? Normally, finding the distance between two array indexes is just simple subtraction; the extra "+1" looks weird.  It's also kind of suspicious that your "zero-based" indexing can return a negative number (as an extreme case, if a 16-bit integer is all ones, your code says the most significant possibly set bit has index -1).
> > > > I'm going to take a look at the actual behavior, I think the least significant possibly set bit might be one-indexed; in any case something is behaving differently than I intended.
> > > > 
> > > > WRT the all-ones case, if a number has no unknown bits then it is by nature a constant, and would be handled by ConstProp, right?
> > > Yes, in the all-ones case it would get turned into a constant; not sure off the top of my head if that's guaranteed to happen before this code runs, though.
> > After checking I remembered the reason for the +1: what I'm calling BitRange is actually the //width//; for example, the width between the 1s in 0b101 would be 3. Maybe width isn't the best word. Whatever the case, this is the reason.
> Oh, hmm... not how I would think of it.
> 
>     b|  1  |  0  |  1  |
>      3     2     1     0
>      high              low
> 
> So the high index is 3, the low index is zero, the significant bits are the bits inside the range, and the length of the range is the high index minus the low index.  This is how iterators and ranges in C++ work.
Right, I hadn't thought of it that way; that is how ranges are expressed in most languages I work with. I'll make the change.


https://reviews.llvm.org/D19178