public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PUSHED] Skip out on processing __builtin_clz when varying.
@ 2021-05-12 21:01 Aldy Hernandez
  2021-05-12 21:08 ` Jakub Jelinek
  0 siblings, 1 reply; 7+ messages in thread
From: Aldy Hernandez @ 2021-05-12 21:01 UTC (permalink / raw)
  To: GCC patches

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

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);
+}
-- 
2.31.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PUSHED] Skip out on processing __builtin_clz when varying.
  2021-05-12 21:01 [PUSHED] Skip out on processing __builtin_clz when varying Aldy Hernandez
@ 2021-05-12 21:08 ` Jakub Jelinek
  2021-05-13 18:00   ` Aldy Hernandez
  2021-05-13 18:01   ` Aldy Hernandez
  0 siblings, 2 replies; 7+ messages in thread
From: Jakub Jelinek @ 2021-05-12 21:08 UTC (permalink / raw)
  To: Aldy Hernandez; +Cc: GCC patches

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PUSHED] Skip out on processing __builtin_clz when varying.
  2021-05-12 21:08 ` Jakub Jelinek
@ 2021-05-13 18:00   ` Aldy Hernandez
  2021-05-27 14:29     ` Aldy Hernandez
  2021-06-03 16:24     ` Fwd: " Aldy Hernandez
  2021-05-13 18:01   ` Aldy Hernandez
  1 sibling, 2 replies; 7+ messages in thread
From: Aldy Hernandez @ 2021-05-13 18:00 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: GCC patches

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



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: text/x-patch, Size: 2568 bytes --]

From f8a958e8028ed129558f9ad7ccf423c834d377bd Mon Sep 17 00:00:00 2001
From: Aldy Hernandez <aldyh@redhat.com>
Date: Thu, 13 May 2021 13:47:41 -0400
Subject: [PATCH] Cleanup clz and ctz code in range_of_builtin_call.

gcc/ChangeLog:

	* gimple-range.cc (range_of_builtin_call): Cleanup clz and ctz
	code.
---
 gcc/gimple-range.cc | 43 ++++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
index 5b288d8e6a7..b33ba1c8099 100644
--- a/gcc/gimple-range.cc
+++ b/gcc/gimple-range.cc
@@ -736,33 +736,29 @@ 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 () && !r.varying_p ())
+      if (!r.undefined_p ())
 	{
-	  int newmaxi = prec - 1 - wi::floor_log2 (r.lower_bound ());
-	  // Argument is unsigned, so do nothing if it is [0, ...] range.
-	  if (newmaxi != prec)
+	  // From clz of minimum we can compute result maximum.
+	  if (wi::gt_p (r.lower_bound (), 0, TYPE_SIGN (r.type ())))
+	    {
+	      maxi = prec - 1 - wi::floor_log2 (r.lower_bound ());
+	      if (mini == -2)
+		mini = 0;
+	    }
+	  else if (!range_includes_zero_p (&r))
 	    {
 	      mini = 0;
-	      maxi = newmaxi;
+	      maxi = prec - 1;
 	    }
-	}
-      else if (!range_includes_zero_p (&r))
-	{
-	  maxi = prec - 1;
-	  mini = 0;
-	}
-      if (mini == -2)
-	break;
-      // From clz of maximum we can compute result minimum.
-      if (r.constant_p ())
-	{
-	  int newmini = prec - 1 - wi::floor_log2 (r.upper_bound ());
-	  if (newmini == prec)
+	  if (mini == -2)
+	    break;
+	  // From clz of maximum we can compute result minimum.
+	  wide_int max = r.upper_bound ();
+	  int newmini = prec - 1 - wi::floor_log2 (max);
+	  if (max == 0)
 	    {
-	      // Argument range is [0, 0].  If CLZ_DEFINED_VALUE_AT_ZERO
-	      // is 2 with VALUE of prec, return [prec, prec], otherwise
-	      // ignore the range.
+	      // If CLZ_DEFINED_VALUE_AT_ZERO is 2 with VALUE of prec,
+	      // return [prec, prec], otherwise ignore the range.
 	      if (maxi == prec)
 		mini = prec;
 	    }
@@ -803,7 +799,8 @@ range_of_builtin_call (range_query &query, irange &r, gcall *call)
       query.range_of_expr (r, arg, call);
       if (!r.undefined_p ())
 	{
-	  if (r.lower_bound () != 0)
+	  // If arg is non-zero, then use [0, prec - 1].
+	  if (!range_includes_zero_p (&r))
 	    {
 	      mini = 0;
 	      maxi = prec - 1;
-- 
2.31.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PUSHED] Skip out on processing __builtin_clz when varying.
  2021-05-12 21:08 ` Jakub Jelinek
  2021-05-13 18:00   ` Aldy Hernandez
@ 2021-05-13 18:01   ` Aldy Hernandez
  1 sibling, 0 replies; 7+ messages in thread
From: Aldy Hernandez @ 2021-05-13 18:01 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: GCC patches

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



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: text/x-patch, Size: 2568 bytes --]

From f8a958e8028ed129558f9ad7ccf423c834d377bd Mon Sep 17 00:00:00 2001
From: Aldy Hernandez <aldyh@redhat.com>
Date: Thu, 13 May 2021 13:47:41 -0400
Subject: [PATCH] Cleanup clz and ctz code in range_of_builtin_call.

gcc/ChangeLog:

	* gimple-range.cc (range_of_builtin_call): Cleanup clz and ctz
	code.
---
 gcc/gimple-range.cc | 43 ++++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
index 5b288d8e6a7..b33ba1c8099 100644
--- a/gcc/gimple-range.cc
+++ b/gcc/gimple-range.cc
@@ -736,33 +736,29 @@ 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 () && !r.varying_p ())
+      if (!r.undefined_p ())
 	{
-	  int newmaxi = prec - 1 - wi::floor_log2 (r.lower_bound ());
-	  // Argument is unsigned, so do nothing if it is [0, ...] range.
-	  if (newmaxi != prec)
+	  // From clz of minimum we can compute result maximum.
+	  if (wi::gt_p (r.lower_bound (), 0, TYPE_SIGN (r.type ())))
+	    {
+	      maxi = prec - 1 - wi::floor_log2 (r.lower_bound ());
+	      if (mini == -2)
+		mini = 0;
+	    }
+	  else if (!range_includes_zero_p (&r))
 	    {
 	      mini = 0;
-	      maxi = newmaxi;
+	      maxi = prec - 1;
 	    }
-	}
-      else if (!range_includes_zero_p (&r))
-	{
-	  maxi = prec - 1;
-	  mini = 0;
-	}
-      if (mini == -2)
-	break;
-      // From clz of maximum we can compute result minimum.
-      if (r.constant_p ())
-	{
-	  int newmini = prec - 1 - wi::floor_log2 (r.upper_bound ());
-	  if (newmini == prec)
+	  if (mini == -2)
+	    break;
+	  // From clz of maximum we can compute result minimum.
+	  wide_int max = r.upper_bound ();
+	  int newmini = prec - 1 - wi::floor_log2 (max);
+	  if (max == 0)
 	    {
-	      // Argument range is [0, 0].  If CLZ_DEFINED_VALUE_AT_ZERO
-	      // is 2 with VALUE of prec, return [prec, prec], otherwise
-	      // ignore the range.
+	      // If CLZ_DEFINED_VALUE_AT_ZERO is 2 with VALUE of prec,
+	      // return [prec, prec], otherwise ignore the range.
 	      if (maxi == prec)
 		mini = prec;
 	    }
@@ -803,7 +799,8 @@ range_of_builtin_call (range_query &query, irange &r, gcall *call)
       query.range_of_expr (r, arg, call);
       if (!r.undefined_p ())
 	{
-	  if (r.lower_bound () != 0)
+	  // If arg is non-zero, then use [0, prec - 1].
+	  if (!range_includes_zero_p (&r))
 	    {
 	      mini = 0;
 	      maxi = prec - 1;
-- 
2.31.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PUSHED] Skip out on processing __builtin_clz when varying.
  2021-05-13 18:00   ` Aldy Hernandez
@ 2021-05-27 14:29     ` Aldy Hernandez
  2021-06-03 16:24     ` Fwd: " Aldy Hernandez
  1 sibling, 0 replies; 7+ messages in thread
From: Aldy Hernandez @ 2021-05-27 14:29 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: GCC patches

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Fwd: [PUSHED] Skip out on processing __builtin_clz when varying.
  2021-05-13 18:00   ` Aldy Hernandez
  2021-05-27 14:29     ` Aldy Hernandez
@ 2021-06-03 16:24     ` Aldy Hernandez
  2021-06-17 10:12       ` Aldy Hernandez
  1 sibling, 1 reply; 7+ messages in thread
From: Aldy Hernandez @ 2021-06-03 16:24 UTC (permalink / raw)
  To: gcc-patches

[-- 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 --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PUSHED] Skip out on processing __builtin_clz when varying.
  2021-06-03 16:24     ` Fwd: " Aldy Hernandez
@ 2021-06-17 10:12       ` Aldy Hernandez
  0 siblings, 0 replies; 7+ messages in thread
From: Aldy Hernandez @ 2021-06-17 10:12 UTC (permalink / raw)
  To: gcc-patches

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

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

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

From f1555d4013ed3cae2589270436387063d1c2f1a3 Mon Sep 17 00:00:00 2001
From: Aldy Hernandez <aldyh@redhat.com>
Date: Thu, 13 May 2021 13:47:41 -0400
Subject: [PATCH] Cleanup clz and ctz code in range_of_builtin_call.

These are various cleanups to the clz/ctz code.

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 PR's testcase is clearly not.  I've cleaned
this up, so that it works either way.

I've also 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.

Tested on x86-64 Linux.

gcc/ChangeLog:

	PR tree-optimization/100790
	* gimple-range.cc (range_of_builtin_call): Cleanup clz and ctz
	code.

gcc/testsuite/ChangeLog:

	* gcc.dg/pr100790.c: New test.
---
 gcc/gimple-range.cc             | 42 ++++++++++++++++-----------------
 gcc/testsuite/gcc.dg/pr100790.c |  4 ++++
 2 files changed, 24 insertions(+), 22 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr100790.c

diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
index 58109701f2f..15c65f16a32 100644
--- a/gcc/gimple-range.cc
+++ b/gcc/gimple-range.cc
@@ -960,32 +960,29 @@ fold_using_range::range_of_builtin_call (irange &r, gcall *call,
 
       src.get_operand (r, arg);
       // From clz of minimum we can compute result maximum.
-      if (r.constant_p () && !r.varying_p ())
+      if (!r.undefined_p ())
 	{
-	  int newmaxi = prec - 1 - wi::floor_log2 (r.lower_bound ());
-	  // Argument is unsigned, so do nothing if it is [0, ...] range.
-	  if (newmaxi != prec)
+	  // From clz of minimum we can compute result maximum.
+	  if (wi::gt_p (r.lower_bound (), 0, TYPE_SIGN (r.type ())))
+	    {
+	      maxi = prec - 1 - wi::floor_log2 (r.lower_bound ());
+	      if (mini == -2)
+		mini = 0;
+	    }
+	  else if (!range_includes_zero_p (&r))
 	    {
 	      mini = 0;
-	      maxi = newmaxi;
+	      maxi = prec - 1;
 	    }
-	}
-      else if (!range_includes_zero_p (&r))
-	{
-	  maxi = prec - 1;
-	  mini = 0;
-	}
-      if (mini == -2)
-	break;
-      // From clz of maximum we can compute result minimum.
-      if (r.constant_p ())
-	{
-	  int newmini = prec - 1 - wi::floor_log2 (r.upper_bound ());
-	  if (newmini == prec)
+	  if (mini == -2)
+	    break;
+	  // From clz of maximum we can compute result minimum.
+	  wide_int max = r.upper_bound ();
+	  int newmini = prec - 1 - wi::floor_log2 (max);
+	  if (max == 0)
 	    {
-	      // Argument range is [0, 0].  If CLZ_DEFINED_VALUE_AT_ZERO
-	      // is 2 with VALUE of prec, return [prec, prec], otherwise
-	      // ignore the range.
+	      // If CLZ_DEFINED_VALUE_AT_ZERO is 2 with VALUE of prec,
+	      // return [prec, prec], otherwise ignore the range.
 	      if (maxi == prec)
 		mini = prec;
 	    }
@@ -1026,7 +1023,8 @@ fold_using_range::range_of_builtin_call (irange &r, gcall *call,
       src.get_operand (r, arg);
       if (!r.undefined_p ())
 	{
-	  if (r.lower_bound () != 0)
+	  // If arg is non-zero, then use [0, prec - 1].
+	  if (!range_includes_zero_p (&r))
 	    {
 	      mini = 0;
 	      maxi = prec - 1;
diff --git a/gcc/testsuite/gcc.dg/pr100790.c b/gcc/testsuite/gcc.dg/pr100790.c
new file mode 100644
index 00000000000..31e0effdea2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr100790.c
@@ -0,0 +1,4 @@
+// { dg-do compile }
+// { dg-options "-O2 -w" }
+
+__builtin_clz(int x) { x ? __builtin_clz(x) : 32; }
-- 
2.31.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-06-17 10:13 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-12 21:01 [PUSHED] Skip out on processing __builtin_clz when varying Aldy Hernandez
2021-05-12 21:08 ` Jakub Jelinek
2021-05-13 18:00   ` Aldy Hernandez
2021-05-27 14:29     ` Aldy Hernandez
2021-06-03 16:24     ` Fwd: " Aldy Hernandez
2021-06-17 10:12       ` Aldy Hernandez
2021-05-13 18:01   ` Aldy Hernandez

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).