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

[Aldy Hernandez <aldyh@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2111 bytes --]

Ping*2

---------- Forwarded message ---------
From: Aldy Hernandez <aldyh@redhat.com>
Date: Thu, May 13, 2021, 20:02
Subject: Re: [PUSHED] Skip out on processing __builtin_clz when varying.
To: Jakub Jelinek <jakub@redhat.com>
Cc: GCC patches <gcc-patches@gcc.gnu.org>




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

[-- Attachment #2: 0001-Cleanup-clz-and-ctz-code-in-range_of_builtin_call.patch --]
[-- Type: application/x-patch, Size: 2650 bytes --]