Re: [PATCH] add VRP for bitwise OR and AND: v3

[Daniel Berlin <dannyb at google dot com>]

Denis Vlasenko wrote:
> Third version of the patch.
> 
> Please review and consider applying.
> 
> I did the testing with 4.1.1, because gcc-4.2-20060805
> does not build for me (I reported the issue separately).
> The changes from 4.2-snapshot are minor, I don't think
> it makes any real difference.
> 
> It looks like operands of binary | and & are already
> casted to the integers or same signedness and width,
> which removed some unneeded complexity from the patch.
> 
> gcc-4.1.1 bootstraps with this patch.
> With the patch, it optimizes out all calls to BADn()
> functions in this code:
> 
> //signed also works
> typedef unsigned number;
> int main(number a,number b,number c) {
>     number r;
>     //VRP won't realize that a and b have ranges
>     //if(a<0x0100 || a>0x0101) return 1;        // 010x
>     //if(b<0x1000 || b>0x1011) return 1;        // 10xx
>     //this works:
>     if(a<0x0100) return 1;  // 010x
>     if(a>0x0101) return 1;  // 010x
>     if(b<0x1000) return 1;  // 10xx
>     if(b>0x1011) return 1;  // 10xx
>     if (a & 0x1000) BAD0();
>     r = a & b;
>     if(r>0x0001) BAD1();
>     r = a | b;
>     if(r<0x1100) BAD2();
>     r = a & c;
>     if(r>0x0101) BAD4();
>     return r;
> }
> 
> On Wednesday 09 August 2006 20:09, Daniel Berlin wrote:
>> Just some comments on the code itself for now (again, you will
>> eventually need a changelog before this patch can be truly reviewed, i
>> would suggest you submit it with the next iteration of this patch, just
> 
> Does this mean that patch has to include changes to gcc/ChangeLog file?

>From the contributing page:

ChangeLog
    A ChangeLog entry as plaintext; see the various ChangeLog files for
format and content, and the GCC coding conventions and GNU Coding
Standards for further information. The ChangeLog entries should be
plaintext rather than part of the patch since the top of the ChangeLog
changes rapidly and a patch to the ChangeLog would probably no longer
apply by the time your patch is reviewed. If your change fixes a PR, put
text in the ChangeLog entry mentioning the PR. The svn commit machinery
understands how to extract this information and automatically append the
commit log to the PR. In order to be recognized, the text must fit a
particular form. It must start with "PR", and then must include the
category and PR number. For instance, PR java/2369 is valid. Multiple
PRs can be mentioned in a single message.

The coding conventions have the exact changelog format if you want to
see it.

> Or what? Anyway:
> 
> * tree-vrp.c: deinline vrp_int_const_binop, it is too large for that

You will want to submit this change separately, and provide some real
justification performance wise.  Generally we don't change whether a
function is static inline or not from what it was originally without
performance numbers, because in most cases, it just doesn't matter.

> * tree-vrp.c (extract_range_from_binary_expr): add value range propagation
>   for bitwise and/or

should be "Add value range propagation for bitwise AND/OR."