expr_not_equal_to: use value_range API

Message ID 8b43336d-d61b-d676-aaa7-14e72530aaa9@redhat.com
State New
Headers show
Series
  • expr_not_equal_to: use value_range API
Related show

Commit Message

Aldy Hernandez Nov. 8, 2018, 12:08 p.m.
All this nonsense:

-      rtype = get_range_info (t, &min, &max);
-      if (rtype == VR_RANGE)
-	{
-	  if (wi::lt_p (max, w, TYPE_SIGN (TREE_TYPE (t))))
-	    return true;
-	  if (wi::lt_p (w, min, TYPE_SIGN (TREE_TYPE (t))))
-	    return true;
-	}
-      else if (rtype == VR_ANTI_RANGE
-	       && wi::le_p (min, w, TYPE_SIGN (TREE_TYPE (t)))
-	       && wi::le_p (w, max, TYPE_SIGN (TREE_TYPE (t))))

Replaced by an API like Kutulu intended.

+      get_range_info (t, vr);
+      if (!vr.may_contain_p (wide_int_to_tree (TREE_TYPE (t), w)))

Ain't it grand?

OK for trunk, depending on get_range_info changes of course?

Aldy

Comments

Richard Biener Nov. 8, 2018, 2:21 p.m. | #1
On Thu, Nov 8, 2018 at 1:09 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>

> All this nonsense:

>

> -      rtype = get_range_info (t, &min, &max);

> -      if (rtype == VR_RANGE)

> -       {

> -         if (wi::lt_p (max, w, TYPE_SIGN (TREE_TYPE (t))))

> -           return true;

> -         if (wi::lt_p (w, min, TYPE_SIGN (TREE_TYPE (t))))

> -           return true;

> -       }

> -      else if (rtype == VR_ANTI_RANGE

> -              && wi::le_p (min, w, TYPE_SIGN (TREE_TYPE (t)))

> -              && wi::le_p (w, max, TYPE_SIGN (TREE_TYPE (t))))

>

> Replaced by an API like Kutulu intended.

>

> +      get_range_info (t, vr);

> +      if (!vr.may_contain_p (wide_int_to_tree (TREE_TYPE (t), w)))

>

> Ain't it grand?


Well.  The not-so-grand thing is that you possibly ggc-allocate
three INTEGER_CST nodes here.

So, no ...?

Shouldn't this instead use wide-int-range.h?  (yeah, there's
the class-ification still missing there)

Richard.

> OK for trunk, depending on get_range_info changes of course?

>

> Aldy
Aldy Hernandez Nov. 8, 2018, 2:27 p.m. | #2
On 11/8/18 9:21 AM, Richard Biener wrote:
> On Thu, Nov 8, 2018 at 1:09 PM Aldy Hernandez <aldyh@redhat.com> wrote:

>>

>> All this nonsense:

>>

>> -      rtype = get_range_info (t, &min, &max);

>> -      if (rtype == VR_RANGE)

>> -       {

>> -         if (wi::lt_p (max, w, TYPE_SIGN (TREE_TYPE (t))))

>> -           return true;

>> -         if (wi::lt_p (w, min, TYPE_SIGN (TREE_TYPE (t))))

>> -           return true;

>> -       }

>> -      else if (rtype == VR_ANTI_RANGE

>> -              && wi::le_p (min, w, TYPE_SIGN (TREE_TYPE (t)))

>> -              && wi::le_p (w, max, TYPE_SIGN (TREE_TYPE (t))))

>>

>> Replaced by an API like Kutulu intended.

>>

>> +      get_range_info (t, vr);

>> +      if (!vr.may_contain_p (wide_int_to_tree (TREE_TYPE (t), w)))

>>

>> Ain't it grand?

> 

> Well.  The not-so-grand thing is that you possibly ggc-allocate

> three INTEGER_CST nodes here.


Hmmm... I'd really prefer to use a simple API call, instead of having to 
twiddle with the extremes manually.  Ideally no one should be looking 
inside of a value_range.

Do recommend another way of implementing may_contain_p ?

Aldy

> 

> So, no ...?

> 

> Shouldn't this instead use wide-int-range.h?  (yeah, there's

> the class-ification still missing there)

> 

> Richard.

> 

>> OK for trunk, depending on get_range_info changes of course?

>>

>> Aldy
Richard Biener Nov. 8, 2018, 2:43 p.m. | #3
On Thu, Nov 8, 2018 at 3:27 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>

>

>

> On 11/8/18 9:21 AM, Richard Biener wrote:

> > On Thu, Nov 8, 2018 at 1:09 PM Aldy Hernandez <aldyh@redhat.com> wrote:

> >>

> >> All this nonsense:

> >>

> >> -      rtype = get_range_info (t, &min, &max);

> >> -      if (rtype == VR_RANGE)

> >> -       {

> >> -         if (wi::lt_p (max, w, TYPE_SIGN (TREE_TYPE (t))))

> >> -           return true;

> >> -         if (wi::lt_p (w, min, TYPE_SIGN (TREE_TYPE (t))))

> >> -           return true;

> >> -       }

> >> -      else if (rtype == VR_ANTI_RANGE

> >> -              && wi::le_p (min, w, TYPE_SIGN (TREE_TYPE (t)))

> >> -              && wi::le_p (w, max, TYPE_SIGN (TREE_TYPE (t))))

> >>

> >> Replaced by an API like Kutulu intended.

> >>

> >> +      get_range_info (t, vr);

> >> +      if (!vr.may_contain_p (wide_int_to_tree (TREE_TYPE (t), w)))

> >>

> >> Ain't it grand?

> >

> > Well.  The not-so-grand thing is that you possibly ggc-allocate

> > three INTEGER_CST nodes here.

>

> Hmmm... I'd really prefer to use a simple API call, instead of having to

> twiddle with the extremes manually.  Ideally no one should be looking

> inside of a value_range.

>

> Do recommend another way of implementing may_contain_p ?


I think many places dealing with get_range_info () should instead
work on the (to be created...) 1:1 copy of value_range ontop of
wide-int-range instead.

So - can you add a wide_int_range class to wide-int-range.h
that implements the same (but with wide-ints rather than trees
obviously) API as value-range?

IIRC you once had sth like that to simpify arg passing to the
workers but otherwise without much functionality?

Richard.

> Aldy

>

> >

> > So, no ...?

> >

> > Shouldn't this instead use wide-int-range.h?  (yeah, there's

> > the class-ification still missing there)

> >

> > Richard.

> >

> >> OK for trunk, depending on get_range_info changes of course?

> >>

> >> Aldy
Aldy Hernandez Nov. 8, 2018, 2:50 p.m. | #4
On 11/8/18 9:43 AM, Richard Biener wrote:
> On Thu, Nov 8, 2018 at 3:27 PM Aldy Hernandez <aldyh@redhat.com> wrote:

>>

>>

>>

>> On 11/8/18 9:21 AM, Richard Biener wrote:

>>> On Thu, Nov 8, 2018 at 1:09 PM Aldy Hernandez <aldyh@redhat.com> wrote:

>>>>

>>>> All this nonsense:

>>>>

>>>> -      rtype = get_range_info (t, &min, &max);

>>>> -      if (rtype == VR_RANGE)

>>>> -       {

>>>> -         if (wi::lt_p (max, w, TYPE_SIGN (TREE_TYPE (t))))

>>>> -           return true;

>>>> -         if (wi::lt_p (w, min, TYPE_SIGN (TREE_TYPE (t))))

>>>> -           return true;

>>>> -       }

>>>> -      else if (rtype == VR_ANTI_RANGE

>>>> -              && wi::le_p (min, w, TYPE_SIGN (TREE_TYPE (t)))

>>>> -              && wi::le_p (w, max, TYPE_SIGN (TREE_TYPE (t))))

>>>>

>>>> Replaced by an API like Kutulu intended.

>>>>

>>>> +      get_range_info (t, vr);

>>>> +      if (!vr.may_contain_p (wide_int_to_tree (TREE_TYPE (t), w)))

>>>>

>>>> Ain't it grand?

>>>

>>> Well.  The not-so-grand thing is that you possibly ggc-allocate

>>> three INTEGER_CST nodes here.

>>

>> Hmmm... I'd really prefer to use a simple API call, instead of having to

>> twiddle with the extremes manually.  Ideally no one should be looking

>> inside of a value_range.

>>

>> Do recommend another way of implementing may_contain_p ?

> 

> I think many places dealing with get_range_info () should instead

> work on the (to be created...) 1:1 copy of value_range ontop of

> wide-int-range instead.


I'd prefer to not expose that we're going to use wide_int or any other 
implementation to the users of get_range_info().

> 

> So - can you add a wide_int_range class to wide-int-range.h

> that implements the same (but with wide-ints rather than trees

> obviously) API as value-range?


Hmmm, I don't have time for this release cycle.  Perhaps something to be 
entertained for GCC+1?

Again, I prefer my patch as is.  I cleans up the code, and keeps us from 
introducing problematic bugs.  Anything dealing with anti ranges is 
fraught with peril, as my cleanups to tree-vrp revealed.

If using these INTEGER_CST's causes any measurable performance 
difference, I'd be happy to look into it.

Aldy
Richard Biener Nov. 8, 2018, 2:55 p.m. | #5
On Thu, Nov 8, 2018 at 3:50 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>

>

>

> On 11/8/18 9:43 AM, Richard Biener wrote:

> > On Thu, Nov 8, 2018 at 3:27 PM Aldy Hernandez <aldyh@redhat.com> wrote:

> >>

> >>

> >>

> >> On 11/8/18 9:21 AM, Richard Biener wrote:

> >>> On Thu, Nov 8, 2018 at 1:09 PM Aldy Hernandez <aldyh@redhat.com> wrote:

> >>>>

> >>>> All this nonsense:

> >>>>

> >>>> -      rtype = get_range_info (t, &min, &max);

> >>>> -      if (rtype == VR_RANGE)

> >>>> -       {

> >>>> -         if (wi::lt_p (max, w, TYPE_SIGN (TREE_TYPE (t))))

> >>>> -           return true;

> >>>> -         if (wi::lt_p (w, min, TYPE_SIGN (TREE_TYPE (t))))

> >>>> -           return true;

> >>>> -       }

> >>>> -      else if (rtype == VR_ANTI_RANGE

> >>>> -              && wi::le_p (min, w, TYPE_SIGN (TREE_TYPE (t)))

> >>>> -              && wi::le_p (w, max, TYPE_SIGN (TREE_TYPE (t))))

> >>>>

> >>>> Replaced by an API like Kutulu intended.

> >>>>

> >>>> +      get_range_info (t, vr);

> >>>> +      if (!vr.may_contain_p (wide_int_to_tree (TREE_TYPE (t), w)))

> >>>>

> >>>> Ain't it grand?

> >>>

> >>> Well.  The not-so-grand thing is that you possibly ggc-allocate

> >>> three INTEGER_CST nodes here.

> >>

> >> Hmmm... I'd really prefer to use a simple API call, instead of having to

> >> twiddle with the extremes manually.  Ideally no one should be looking

> >> inside of a value_range.

> >>

> >> Do recommend another way of implementing may_contain_p ?

> >

> > I think many places dealing with get_range_info () should instead

> > work on the (to be created...) 1:1 copy of value_range ontop of

> > wide-int-range instead.

>

> I'd prefer to not expose that we're going to use wide_int or any other

> implementation to the users of get_range_info().


But it's exposed at the moment.  And I don't see it change.  And you
should not allocate memory for no good reason.

> >

> > So - can you add a wide_int_range class to wide-int-range.h

> > that implements the same (but with wide-ints rather than trees

> > obviously) API as value-range?

>

> Hmmm, I don't have time for this release cycle.  Perhaps something to be

> entertained for GCC+1?

>

> Again, I prefer my patch as is.  I cleans up the code, and keeps us from

> introducing problematic bugs.  Anything dealing with anti ranges is

> fraught with peril, as my cleanups to tree-vrp revealed.

>

> If using these INTEGER_CST's causes any measurable performance

> difference, I'd be happy to look into it.


Just leave the code unchanged then in this release?

Richard.

> Aldy
Aldy Hernandez Nov. 8, 2018, 3:31 p.m. | #6
On 11/8/18 9:55 AM, Richard Biener wrote:
> On Thu, Nov 8, 2018 at 3:50 PM Aldy Hernandez <aldyh@redhat.com> wrote:

>>

>>

>>

>> On 11/8/18 9:43 AM, Richard Biener wrote:

>>> On Thu, Nov 8, 2018 at 3:27 PM Aldy Hernandez <aldyh@redhat.com> wrote:

>>>>

>>>>

>>>>

>>>> On 11/8/18 9:21 AM, Richard Biener wrote:

>>>>> On Thu, Nov 8, 2018 at 1:09 PM Aldy Hernandez <aldyh@redhat.com> wrote:

>>>>>>

>>>>>> All this nonsense:

>>>>>>

>>>>>> -      rtype = get_range_info (t, &min, &max);

>>>>>> -      if (rtype == VR_RANGE)

>>>>>> -       {

>>>>>> -         if (wi::lt_p (max, w, TYPE_SIGN (TREE_TYPE (t))))

>>>>>> -           return true;

>>>>>> -         if (wi::lt_p (w, min, TYPE_SIGN (TREE_TYPE (t))))

>>>>>> -           return true;

>>>>>> -       }

>>>>>> -      else if (rtype == VR_ANTI_RANGE

>>>>>> -              && wi::le_p (min, w, TYPE_SIGN (TREE_TYPE (t)))

>>>>>> -              && wi::le_p (w, max, TYPE_SIGN (TREE_TYPE (t))))

>>>>>>

>>>>>> Replaced by an API like Kutulu intended.

>>>>>>

>>>>>> +      get_range_info (t, vr);

>>>>>> +      if (!vr.may_contain_p (wide_int_to_tree (TREE_TYPE (t), w)))

>>>>>>

>>>>>> Ain't it grand?

>>>>>

>>>>> Well.  The not-so-grand thing is that you possibly ggc-allocate

>>>>> three INTEGER_CST nodes here.

>>>>

>>>> Hmmm... I'd really prefer to use a simple API call, instead of having to

>>>> twiddle with the extremes manually.  Ideally no one should be looking

>>>> inside of a value_range.

>>>>

>>>> Do recommend another way of implementing may_contain_p ?

>>>

>>> I think many places dealing with get_range_info () should instead

>>> work on the (to be created...) 1:1 copy of value_range ontop of

>>> wide-int-range instead.

>>

>> I'd prefer to not expose that we're going to use wide_int or any other

>> implementation to the users of get_range_info().

> 

> But it's exposed at the moment.  And I don't see it change.  And you

> should not allocate memory for no good reason.

> 

>>>

>>> So - can you add a wide_int_range class to wide-int-range.h

>>> that implements the same (but with wide-ints rather than trees

>>> obviously) API as value-range?

>>

>> Hmmm, I don't have time for this release cycle.  Perhaps something to be

>> entertained for GCC+1?

>>

>> Again, I prefer my patch as is.  I cleans up the code, and keeps us from

>> introducing problematic bugs.  Anything dealing with anti ranges is

>> fraught with peril, as my cleanups to tree-vrp revealed.

>>

>> If using these INTEGER_CST's causes any measurable performance

>> difference, I'd be happy to look into it.

> 

> Just leave the code unchanged then in this release?


Ok.

Patch

commit 3a3fa7eb1baba60d17b4b7731972951173c5d615
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Thu Nov 8 13:04:59 2018 +0100

            * fold-const.c (expr_not_equal_to): Use value_range API.

diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 5399288dfc5..744f946fa15 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -9255,8 +9255,7 @@  tree_expr_nonzero_p (tree t)
 bool
 expr_not_equal_to (tree t, const wide_int &w)
 {
-  wide_int min, max, nz;
-  value_range_kind rtype;
+  value_range vr;
   switch (TREE_CODE (t))
     {
     case INTEGER_CST:
@@ -9265,17 +9264,8 @@  expr_not_equal_to (tree t, const wide_int &w)
     case SSA_NAME:
       if (!INTEGRAL_TYPE_P (TREE_TYPE (t)))
 	return false;
-      rtype = get_range_info (t, &min, &max);
-      if (rtype == VR_RANGE)
-	{
-	  if (wi::lt_p (max, w, TYPE_SIGN (TREE_TYPE (t))))
-	    return true;
-	  if (wi::lt_p (w, min, TYPE_SIGN (TREE_TYPE (t))))
-	    return true;
-	}
-      else if (rtype == VR_ANTI_RANGE
-	       && wi::le_p (min, w, TYPE_SIGN (TREE_TYPE (t)))
-	       && wi::le_p (w, max, TYPE_SIGN (TREE_TYPE (t))))
+      get_range_info (t, vr);
+      if (!vr.may_contain_p (wide_int_to_tree (TREE_TYPE (t), w)))
 	return true;
       /* If T has some known zero bits and W has any of those bits set,
 	 then T is known not to be equal to W.  */