diff mbox series

[PUSHED] Skip out on processing __builtin_clz when varying.

Message ID 20210512210059.301816-1-aldyh@redhat.com
State New
Headers show
Series [PUSHED] Skip out on processing __builtin_clz when varying. | expand

Commit Message

Aldy Hernandez May 12, 2021, 9:01 p.m. UTC 
The previous changes to irange::constant_p return TRUE for
VARYING, since VARYING has numerical end points like any other
constant range.  The problem is that some users of constant_p
depended on constant_p excluding the full domain.  The
range handler for __builtin_clz, that is shared between ranger
and vr_values, is one such user.

This patch excludes varying_p(), to match the original behavior
for clz.

Tested on x86-64 Linux.

gcc/ChangeLog:

	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

Comments

Jakub Jelinek May 12, 2021, 9:08 p.m. UTC | #1 
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...

> +{
> +  return __builtin_clz(a);
> +}
> -- 
> 2.31.1

	Jakub
Aldy Hernandez May 13, 2021, 6 p.m. UTC | #2 
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
Aldy Hernandez May 13, 2021, 6:01 p.m. UTC | #3 
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
Aldy Hernandez May 27, 2021, 2:29 p.m. UTC | #4 
PING

On Thu, May 13, 2021 at 8:02 PM Aldy Hernandez <aldyh@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
Aldy Hernandez June 3, 2021, 4:24 p.m. UTC | #5 
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
Aldy Hernandez June 17, 2021, 10:12 a.m. UTC | #6 
I am pushing this with my new super powers.  It also fixes PR100790,
which is a plus.

Final version of patch attached.
Aldy

On Thu, Jun 3, 2021 at 6:24 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> 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
diff mbox series

Patch

diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
index e94bb355de3..5b288d8e6a7 100644
--- a/gcc/gimple-range.cc
+++ b/gcc/gimple-range.cc
@@ -737,7 +737,7 @@  range_of_builtin_call (range_query &query, irange &r, gcall *call)
 
       query.range_of_expr (r, arg, call);
       // From clz of minimum we can compute result maximum.
-      if (r.constant_p ())
+      if (r.constant_p () && !r.varying_p ())
 	{
 	  int newmaxi = prec - 1 - wi::floor_log2 (r.lower_bound ());
 	  // Argument is unsigned, so do nothing if it is [0, ...] range.
diff --git a/gcc/testsuite/gcc.dg/pr100521.c b/gcc/testsuite/gcc.dg/pr100521.c
new file mode 100644
index 00000000000..fd9f0db5412
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr100521.c
@@ -0,0 +1,8 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+int
+__builtin_clz (int a)
+{
+  return __builtin_clz(a);
+}