implement value_range::domain_p()

Message ID b63b775a-d313-f9b1-74f9-58181708732b@redhat.com
State New
Headers show
Series
  • implement value_range::domain_p()
Related show

Commit Message

Aldy Hernandez Nov. 8, 2018, 12:51 p.m.
I believe I've seen this idiom more than once.  I know for sure I've 
used it in our ssa-range branch :).  I'll hunt for the other uses and 
adjust accordingly.

OK?

Comments

Richard Biener Nov. 8, 2018, 2:34 p.m. | #1
On Thu, Nov 8, 2018 at 1:52 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> I believe I've seen this idiom more than once.  I know for sure I've
> used it in our ssa-range branch :).  I'll hunt for the other uses and
> adjust accordingly.

domain_p?!  Isn't that the same as varying_p()?  Also

+  if (m_kind == VR_RANGE)
+    {
+      tree type = TREE_TYPE (type ());
+      value_range vr (VR_RANGE, TYPE_MIN_VALUE (type), TYPE_MAX_VALUE (type));

TYPE_MIN/MAX_VALUE is NULL for pointers

+      return equal_p (vr, /*ignore_equivs=*/true);

But equivs essentially are making the range smaller!  The range
is the intersection of itself with all equiv ranges!

Richard.

+    }


> OK?
Aldy Hernandez Nov. 8, 2018, 2:40 p.m. | #2
On 11/8/18 9:34 AM, Richard Biener wrote:
> On Thu, Nov 8, 2018 at 1:52 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>
>> I believe I've seen this idiom more than once.  I know for sure I've
>> used it in our ssa-range branch :).  I'll hunt for the other uses and
>> adjust accordingly.
> 
> domain_p?!  Isn't that the same as varying_p()?  Also

Sigh, yes.  If we kept normalized value_ranges around where [MIN,MAX] 
degraded into VR_VARYING, then yes.  But alas, we have two ways of 
representing the entire domain.  Don't look at me.  That crap was 
already there :).

Another approach would be to call ::set_and_canonicalize() before 
checking varying_p() and teach the canonicalize function that [MIN, MAX] 
is VR_VARYING.  How does that sound?

Aldy

> 
> +  if (m_kind == VR_RANGE)
> +    {
> +      tree type = TREE_TYPE (type ());
> +      value_range vr (VR_RANGE, TYPE_MIN_VALUE (type), TYPE_MAX_VALUE (type));
> 
> TYPE_MIN/MAX_VALUE is NULL for pointers
> 
> +      return equal_p (vr, /*ignore_equivs=*/true);
> 
> But equivs essentially are making the range smaller!  The range
> is the intersection of itself with all equiv ranges!
> 
> Richard.
> 
> +    }
> 
> 
>> OK?
Richard Biener Nov. 8, 2018, 2:53 p.m. | #3
On Thu, Nov 8, 2018 at 3:40 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
>
>
> On 11/8/18 9:34 AM, Richard Biener wrote:
> > On Thu, Nov 8, 2018 at 1:52 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >>
> >> I believe I've seen this idiom more than once.  I know for sure I've
> >> used it in our ssa-range branch :).  I'll hunt for the other uses and
> >> adjust accordingly.
> >
> > domain_p?!  Isn't that the same as varying_p()?  Also
>
> Sigh, yes.  If we kept normalized value_ranges around where [MIN,MAX]
> degraded into VR_VARYING, then yes.  But alas, we have two ways of
> representing the entire domain.  Don't look at me.  That crap was
> already there :).
>
> Another approach would be to call ::set_and_canonicalize() before
> checking varying_p() and teach the canonicalize function that [MIN, MAX]
> is VR_VARYING.  How does that sound?

But that's still broken for the case where it has equivalences.  I fear that
if you look at users we'll end up with three or more different
varying_p/domain_p
things all being subtly different...

As said in the other thread we should see to separate equivs out
of the way.

> Aldy
>
> >
> > +  if (m_kind == VR_RANGE)
> > +    {
> > +      tree type = TREE_TYPE (type ());
> > +      value_range vr (VR_RANGE, TYPE_MIN_VALUE (type), TYPE_MAX_VALUE (type));
> >
> > TYPE_MIN/MAX_VALUE is NULL for pointers
> >
> > +      return equal_p (vr, /*ignore_equivs=*/true);
> >
> > But equivs essentially are making the range smaller!  The range
> > is the intersection of itself with all equiv ranges!
> >
> > Richard.
> >
> > +    }
> >
> >
> >> OK?
Aldy Hernandez Nov. 8, 2018, 3:05 p.m. | #4
On 11/8/18 9:53 AM, Richard Biener wrote:
> On Thu, Nov 8, 2018 at 3:40 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>
>>
>>
>> On 11/8/18 9:34 AM, Richard Biener wrote:
>>> On Thu, Nov 8, 2018 at 1:52 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>>>
>>>> I believe I've seen this idiom more than once.  I know for sure I've
>>>> used it in our ssa-range branch :).  I'll hunt for the other uses and
>>>> adjust accordingly.
>>>
>>> domain_p?!  Isn't that the same as varying_p()?  Also
>>
>> Sigh, yes.  If we kept normalized value_ranges around where [MIN,MAX]
>> degraded into VR_VARYING, then yes.  But alas, we have two ways of
>> representing the entire domain.  Don't look at me.  That crap was
>> already there :).
>>
>> Another approach would be to call ::set_and_canonicalize() before
>> checking varying_p() and teach the canonicalize function that [MIN, MAX]
>> is VR_VARYING.  How does that sound?
> 
> But that's still broken for the case where it has equivalences.  I fear that
> if you look at users we'll end up with three or more different
> varying_p/domain_p
> things all being subtly different...

Bah, I give up.  I don't have time to look into possible subtleties wrt 
equivalences right now.  I'll drop this patch.

> 
> As said in the other thread we should see to separate equivs out
> of the way.

And as I meant to say in the other thread, I'll buy you many beers if 
you can do this for this release :).

Aldy
Richard Biener Nov. 8, 2018, 3:24 p.m. | #5
On Thu, Nov 8, 2018 at 4:05 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
>
>
> On 11/8/18 9:53 AM, Richard Biener wrote:
> > On Thu, Nov 8, 2018 at 3:40 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >>
> >>
> >>
> >> On 11/8/18 9:34 AM, Richard Biener wrote:
> >>> On Thu, Nov 8, 2018 at 1:52 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >>>>
> >>>> I believe I've seen this idiom more than once.  I know for sure I've
> >>>> used it in our ssa-range branch :).  I'll hunt for the other uses and
> >>>> adjust accordingly.
> >>>
> >>> domain_p?!  Isn't that the same as varying_p()?  Also
> >>
> >> Sigh, yes.  If we kept normalized value_ranges around where [MIN,MAX]
> >> degraded into VR_VARYING, then yes.  But alas, we have two ways of
> >> representing the entire domain.  Don't look at me.  That crap was
> >> already there :).
> >>
> >> Another approach would be to call ::set_and_canonicalize() before
> >> checking varying_p() and teach the canonicalize function that [MIN, MAX]
> >> is VR_VARYING.  How does that sound?
> >
> > But that's still broken for the case where it has equivalences.  I fear that
> > if you look at users we'll end up with three or more different
> > varying_p/domain_p
> > things all being subtly different...
>
> Bah, I give up.  I don't have time to look into possible subtleties wrt
> equivalences right now.  I'll drop this patch.
>
> >
> > As said in the other thread we should see to separate equivs out
> > of the way.
>
> And as I meant to say in the other thread, I'll buy you many beers if
> you can do this for this release :).

Well, yall made a mess out of the nicely contained VRP, and topped
it with C++.

Now it's ... a mess.

And for whatever reason all the C++-ification and refactoring had to happen
for GCC 9 :/

> Aldy
Aldy Hernandez Nov. 8, 2018, 3:30 p.m. | #6
On 11/8/18 10:24 AM, Richard Biener wrote:
> On Thu, Nov 8, 2018 at 4:05 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>
>>
>>
>> On 11/8/18 9:53 AM, Richard Biener wrote:
>>> On Thu, Nov 8, 2018 at 3:40 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>>>
>>>>
>>>>
>>>> On 11/8/18 9:34 AM, Richard Biener wrote:
>>>>> On Thu, Nov 8, 2018 at 1:52 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>>>>>
>>>>>> I believe I've seen this idiom more than once.  I know for sure I've
>>>>>> used it in our ssa-range branch :).  I'll hunt for the other uses and
>>>>>> adjust accordingly.
>>>>>
>>>>> domain_p?!  Isn't that the same as varying_p()?  Also
>>>>
>>>> Sigh, yes.  If we kept normalized value_ranges around where [MIN,MAX]
>>>> degraded into VR_VARYING, then yes.  But alas, we have two ways of
>>>> representing the entire domain.  Don't look at me.  That crap was
>>>> already there :).
>>>>
>>>> Another approach would be to call ::set_and_canonicalize() before
>>>> checking varying_p() and teach the canonicalize function that [MIN, MAX]
>>>> is VR_VARYING.  How does that sound?
>>>
>>> But that's still broken for the case where it has equivalences.  I fear that
>>> if you look at users we'll end up with three or more different
>>> varying_p/domain_p
>>> things all being subtly different...
>>
>> Bah, I give up.  I don't have time to look into possible subtleties wrt
>> equivalences right now.  I'll drop this patch.
>>
>>>
>>> As said in the other thread we should see to separate equivs out
>>> of the way.
>>
>> And as I meant to say in the other thread, I'll buy you many beers if
>> you can do this for this release :).
> 
> Well, yall made a mess out of the nicely contained VRP, and topped
> it with C++.
> 
> Now it's ... a mess.

You wish!  VRP has been a mess for quite a long time.

> 
> And for whatever reason all the C++-ification and refactoring had to happen
> for GCC 9 :/

You're gonna absolutely love what we have in store for GCC 10 ;-).

Aldy
Richard Biener Nov. 9, 2018, 11:31 a.m. | #7
On Thu, Nov 8, 2018 at 4:30 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
>
>
> On 11/8/18 10:24 AM, Richard Biener wrote:
> > On Thu, Nov 8, 2018 at 4:05 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >>
> >>
> >>
> >> On 11/8/18 9:53 AM, Richard Biener wrote:
> >>> On Thu, Nov 8, 2018 at 3:40 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 11/8/18 9:34 AM, Richard Biener wrote:
> >>>>> On Thu, Nov 8, 2018 at 1:52 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >>>>>>
> >>>>>> I believe I've seen this idiom more than once.  I know for sure I've
> >>>>>> used it in our ssa-range branch :).  I'll hunt for the other uses and
> >>>>>> adjust accordingly.
> >>>>>
> >>>>> domain_p?!  Isn't that the same as varying_p()?  Also
> >>>>
> >>>> Sigh, yes.  If we kept normalized value_ranges around where [MIN,MAX]
> >>>> degraded into VR_VARYING, then yes.  But alas, we have two ways of
> >>>> representing the entire domain.  Don't look at me.  That crap was
> >>>> already there :).
> >>>>
> >>>> Another approach would be to call ::set_and_canonicalize() before
> >>>> checking varying_p() and teach the canonicalize function that [MIN, MAX]
> >>>> is VR_VARYING.  How does that sound?
> >>>
> >>> But that's still broken for the case where it has equivalences.  I fear that
> >>> if you look at users we'll end up with three or more different
> >>> varying_p/domain_p
> >>> things all being subtly different...
> >>
> >> Bah, I give up.  I don't have time to look into possible subtleties wrt
> >> equivalences right now.  I'll drop this patch.
> >>
> >>>
> >>> As said in the other thread we should see to separate equivs out
> >>> of the way.
> >>
> >> And as I meant to say in the other thread, I'll buy you many beers if
> >> you can do this for this release :).
> >
> > Well, yall made a mess out of the nicely contained VRP, and topped
> > it with C++.
> >
> > Now it's ... a mess.
>
> You wish!  VRP has been a mess for quite a long time.
>
> >
> > And for whatever reason all the C++-ification and refactoring had to happen
> > for GCC 9 :/
>
> You're gonna absolutely love what we have in store for GCC 10 ;-).

I believe it when I see it.  Oh wait - that was sarcastic!

Richard.

> Aldy

Patch

            * tree-vrp.h (value_range::domain_p): New.
            * tree-vrp.c (value_range::domain_p): New.
            * tree-ssa-strlen.c (strlen_check_and_optimize_stmt): Use
            value_range::domain_p instead of doing things ad-hoc.

diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
index 669c315dce2..ddb61e5a7f3 100644
--- a/gcc/tree-ssa-strlen.c
+++ b/gcc/tree-ssa-strlen.c
@@ -3687,19 +3687,16 @@  strlen_check_and_optimize_stmt (gimple_stmt_iterator *gsi, bool *cleanup_eh)
 			/* Reading a character before the final '\0'
 			   character.  Just set the value range to ~[0, 0]
 			   if we don't have anything better.  */
-			wide_int min, max;
+			value_range vr;
+			get_range_info (lhs, vr);
 			tree type = TREE_TYPE (lhs);
-			enum value_range_kind vr
-			  = get_range_info (lhs, &min, &max);
-			if (vr == VR_VARYING
-			    || (vr == VR_RANGE
-				&& min == wi::min_value (TYPE_PRECISION (type),
-							 TYPE_SIGN (type))
-				&& max == wi::max_value (TYPE_PRECISION (type),
-							 TYPE_SIGN (type))))
-			  set_range_info (lhs, VR_ANTI_RANGE,
-					  wi::zero (TYPE_PRECISION (type)),
-					  wi::zero (TYPE_PRECISION (type)));
+			if (vr.domain_p ())
+			  {
+			    value_range vr (VR_ANTI_RANGE,
+					    build_int_cst (type, 0),
+					    build_int_cst (type, 0));
+			    set_range_info (lhs, vr);
+			  }
 		      }
 		  }
 	      }
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 3ccf2e491d6..e2126c21974 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -178,6 +178,22 @@  value_range::equal_p (const value_range &other, bool ignore_equivs) const
 	      || vrp_bitmap_equal_p (m_equiv, other.m_equiv)));
 }
 
+/* Return TRUE if value_range spans the entire domain.  */
+
+bool
+value_range::domain_p () const
+{
+  if (varying_p ())
+    return true;
+  if (m_kind == VR_RANGE)
+    {
+      tree type = TREE_TYPE (type ());
+      value_range vr (VR_RANGE, TYPE_MIN_VALUE (type), TYPE_MAX_VALUE (type));
+      return equal_p (vr, /*ignore_equivs=*/true);
+    }
+  return false;
+}
+
 /* Return equality while ignoring equivalence bitmap.  */
 
 bool
diff --git a/gcc/tree-vrp.h b/gcc/tree-vrp.h
index c251329a195..c5811d50bbf 100644
--- a/gcc/tree-vrp.h
+++ b/gcc/tree-vrp.h
@@ -64,6 +64,7 @@  class GTY((for_user)) value_range
   /* Misc methods.  */
   tree type () const;
   bool null_p () const;
+  bool domain_p () const;
   bool may_contain_p (tree) const;
   bool singleton_p (tree *result = NULL) const;
   void deep_copy (const value_range *);