Re: [PUSHED] Skip out on processing __builtin_clz when varying.

[Aldy Hernandez via Gcc-patches]

PING

On Thu, May 13, 2021 at 8:02 PM Aldy Hernandez <al...@redhat.com> wrote:
>
>
>
> On 5/12/21 5:08 PM, Jakub Jelinek wrote:
> > On Wed, May 12, 2021 at 05:01:00PM -0400, Aldy Hernandez via Gcc-patches 
> > wrote:
> >>
> >>      PR c/100521
> >>      * gimple-range.cc (range_of_builtin_call): Skip out on
> >>        processing __builtin_clz when varying.
> >> ---
> >>   gcc/gimple-range.cc             | 2 +-
> >>   gcc/testsuite/gcc.dg/pr100521.c | 8 ++++++++
> >>   2 files changed, 9 insertions(+), 1 deletion(-)
> >>   create mode 100644 gcc/testsuite/gcc.dg/pr100521.c
> >>
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.dg/pr100521.c
> >> @@ -0,0 +1,8 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-O2" } */
> >> +
> >> +int
> >> +__builtin_clz (int a)
> >
> > Is this intentional?  People shouldn't be redefining builtins...
>
> Ughhh.  I don't think that's intentional.  For that matter, the current
> nor the old code is designed to deal with this, especially in this case
> when the builtin is being redefined with incompatible arguments.  That
> is, the above "builtin" has a signed integer as an argument, whereas the
> original builtin had an unsigned one.
>
> In looking at the original vr-values code, I think this could use a
> cleanup.  First, ranges from range_of_expr are always numeric so we
> should adjust.  Also, the checks for non-zero were assuming the argument
> was unsigned, which in the above redirect is clearly not.  I've cleaned
> this up, so that it works either way, though perhaps we should _also_
> bail on non-builtins. I don't know...this is before my time.
>
> BTW, I've removed the following annoying idiom:
>
> -         int newmini = prec - 1 - wi::floor_log2 (r.upper_bound ());
> -         if (newmini == prec)
>
> This is really a check for r.upper_bound() == 0, as floor_log2(0)
> returns -1.  It's confusing.
>
> How does this look?  For reference, the original code where this all
> came from is 82b6d25d289195.
>
> Thanks for pointing this out.
> Aldy