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.  */