Re: [RFA] [PATCH] Make VRP understand bitwise OR and AND (better than now)

["Richard Guenther" <richard dot guenther at gmail dot com>]

On Mon, Aug 11, 2008 at 2:14 PM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
> On Mon, 2008-08-04 at 14:26 +0200, Richard Guenther wrote:
>> On Mon, Aug 4, 2008 at 2:02 PM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
>> > This is a patch for bug 28632 "VRP should understand bitwise OR and AND"
>> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28632
>> >
>> > This patch is bootstrapping successfully on current gcc svn.
>> >
>> > bug28632_instrumented.patch is an instrumented version of the patch.
>> > Set $VDA_DEBUG to the name of a file and gcc will append debug printouts to it
>> > showing how it deduced value ranges for (a | b) and (a & b).
>> >
>> > // extract_range_from_binary_expr(OR)[u32]
>> > // a integer_nonnegative_range(unsigned int __bid_IDEC_glbflags.40_536)=0
>> > // b integer_nonnegative_range(_IDEC_flags[u32],[16,16])=1
>> >  if (a | b) < (16) || (a | b) > (4294967295)) err();
>> >
>> > [gcc inferred that since b = 16, (a | b) is always >= 16,
>> > despite the fact we do not know value range of a]
>> >
>> > // extract_range_from_binary_expr(AND)[u32]
>> > // a integer_nonnegative_range(unsigned int[u32],[0,4294967295])=1
>> > // b integer_nonnegative_range(_IDEC_round[u32],[1,1])=1
>> > // bits_always_set(0,4294967295)=[u32]0
>> > // bits_always_set(1,1)=[u32]1
>> > // bits_maybe_set(0,4294967295)=[u32]4294967295
>> > // bits_maybe_set(1,1)=[u32]1
>> >  if (a & b) < 0 || (a & b) > 1 err();
>> >
>> > [a case where both operands had known positive range]
> ...
> ...
>> In extract_range_from_binary_expr it looks like the cases for
>> BIT_AND_EXPR and BIT_IOR_EXPR can share most (if not all) of
>> the code if you use the expression code instead of constant codes.
>>
>> In bits_always_set and bits_maybe_set it is better to use double_ints
>> (see double_int.h) for intermediate calculations, as they do not involve
>> allocating new tree constants.
>
> The updated patch is attached (with instrumentation code present but
> #defined out).

Uh, please remove the instrumentation code - it makes the code hard
to follow.

>> integer_nonnegative_range needs a comment.
>
> Fixed.
>
> Please review attached patch.

This looks much better (with the instrumentation code removed).  Please
add a testcase or two, write a proper ChangeLog and specify where
you bootstrapped and tested the patch.

Thanks,
Richard.