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

[Aldy Hernandez <aldyh@redhat.com>]

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