[PATCH] D21010: Replace the implementation of ConstantRange::binaryAnd.

[Nick Lewycky via llvm-commits]

Nick Lewycky wrote:
> nicholas added a subscriber: nicholas.
> nicholas added a comment.
>
> Sanjoy Das wrote:
>
>> sanjoy added a comment.
>
>>
>
>> Hi Nick,
>
>>
>
>> I haven't read the code yet (will do that soon), but I wonder if you've tried the following approach: map a `ConstantRange` to `KnownOne` and `KnownZero`, use these to compute the binary and, and map the resultant `KnownOne` and `KnownZero` back to a `ConstantRange`.  That will probably not be easier in terms of code, but will make the code organization more obvious, and most of the interesting logic can be re-used for `binaryOr`.
>
>
> That is the very first thing I tried. :-D
>
> I still have the code (and tests!) for working fromKnownBits and
> toKnownBits methods if you want them. I was surprised at how small and
> efficient they were.
>
> Extracting one test which would fail with that implementation:
>
>    EXPECT_EQ(ConstantRange(APInt(8, 21), APInt(8, 25))
>                  .binaryAnd(ConstantRange(APInt(8, 22), APInt(8, 26))),
>              ConstantRange(APInt(8, 16), APInt(8, 25)));
>
> NOTE: Closed ranges below, not half-open!
>
> This is testing i8 [21, 24]&  [22, 25]
>
> 21 0x15 0b10101 LHS only
> 22 0x16 0b10110 LHS and RHS
> 23 0x17 0b10111 LHS and RHS
> 24 0x18 0b11000 LHS and RHS
> 25 0x19 0b11001 RHS only
>
> LHS range has known bits 0b1xxxx and RHS has known bits 0b1xxxx. The
> binary-and of those is 0b1xxxx, which produces the range i8 [16, 31],
> but there is no pair of values which, when and'd, produce 26, 27, 28,
> 29, 30 or 31. You can get 16 by taking 21 from LHS and 24 from RHS, and
> 24 by taking 24 from both ranges. i8 [21, 24] is the correct answer.

I need to strike an example from this paragraph:

> That example worked out too cleanly; the lower was equal to 'choosing 0
> for unknown bits' and the upper-1 was the umin of the unsigned-max's of
> each range. To dispel a few patterns up front, i8 [4, 5]&  [8, 9] is an
> example where the new upper-1 is 1 not 5, while i8 [5, 6]&  [5, 6] has a
> new lower of 4, but i8 [5, 6]&  [5] has a new lower of 5.

"but i8 [5, 6] & [5] has a new lower of 5" is wrong. i8 [5, 6] & [5, 5] 
has a lower of 5&6 = 4.

I'll trade it in for a new example: i8 [152, 155] & [176, 191] has a 
umin of 144.

The result-lower does have to be less than or equal to the min(lhs 
lower, rhs lower), because anding can't produce a value larger than the 
smaller of its inputs. Similarly the result upper-1 is always less than 
or equal to the min of the upper-1's of the ranges.

Nick

> And since I have it on my brain, here's a little more about wrapped
> ranges. I made an error thinking that I could 'unroll' wrapped ranges,
> turning i8 [255, 0] into i9 [255, 256], binary-and those, and then
> truncate the resulting range. Not so! It's correct, but it does not
> produce the smallest range. Correctly:
>
>      i8 [255, 0]&  i8 [255, 0]
>    = i8 {255&255, 255&0, 0&255, 0&0}  (curly braces for sets)
>    = i8 {255, 0, 0, 0}
>    = i8 {255, 0}
>    = i8 [255, 0].
>
> With the unrolling:
>
>    i8 [255, 0]&  i8 [255, 0]
>
> -->  i9 [255, 256]&  i9 [255, 256]
>
>    = i9 {255&255, 255&256, 256&255, 256&256}
>    = i9 {255, 0, 0, 256}  (here be dragons!)
>    = i9 [0, 256]  (this range includes 1 and 2 and 3, ...)
>
> -->  i8 full-set
>
> My current approach gets this particular case right, and I'm pretty sure
> it always produces ranges that cover all resulting values, but I'm not
> sure it always produces the smallest range.
>
> Nick
>
>
> http://reviews.llvm.org/D21010
>
>
>
>