| Message ID | 8b43336d-d61b-d676-aaa7-14e72530aaa9@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | expr_not_equal_to: use value_range API | expand |
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
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
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
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
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
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.
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. */
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