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 *);