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 <al...@redhat.com> wrote:
>
> Ping*2
>
> ---------- Forwarded message ---------
> From: Aldy Hernandez <al...@redhat.com>
> Date: Thu, May 13, 2021, 20:02
> Subject: Re: [PUSHED] Skip out on processing __builtin_clz when varying.
> To: Jakub Jelinek <ja...@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
From f1555d4013ed3cae2589270436387063d1c2f1a3 Mon Sep 17 00:00:00 2001
From: Aldy Hernandez <al...@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
