[prev in list] [next in list] [prev in thread] [next in thread] 

List:       llvm-commits
Subject:    Re: [PATCH] D19178: Broaden FoldItoFPtoI to try and establish whether the integer value fits into th
From:       Eli Friedman via llvm-commits <llvm-commits () lists ! llvm ! org>
Date:       2016-07-31 22:50:38
Message-ID: e8787111924f935fdcb106334caa0695 () localhost ! localdomain
[Download RAW message or body]

eli.friedman added inline comments.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1507
@@ +1506,3 @@
+    // then the conversion is undefined behavior.
+
+    Safe = BitRange <= SignificandWidth;
----------------
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.


https://reviews.llvm.org/D19178



_______________________________________________
llvm-commits mailing list
llvm-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits


[prev in list] [next in list] [prev in thread] [next in thread]