handle symbolics when comparing ranges

Message ID 2fc9b482-d119-8e29-91dc-078664dbeaea@redhat.com
State New
Headers show
Series
  • handle symbolics when comparing ranges
Related show

Commit Message

Aldy Hernandez Nov. 4, 2019, 10:23 p.m.
value_range_base::operator== was originally lifted from a world where 
symbolics didn't exist (the ranger branch), but symbolics do exist in 
mainline.

Although this isn't causing a problem yet, as soon as someone tries to 
compare non numeric ranges, we'll die.  Using operand_equal_p simplifies 
the comparison code drastically.

I suppose if/when we get multiple sub-ranges in value_range_base, we'll 
have to adapt this function again to compare things piece wise.  For 
now, let's keep things simple.

OK pending tests?

Comments

Andrew MacLeod Nov. 4, 2019, 10:45 p.m. | #1
On 11/4/19 5:23 PM, Aldy Hernandez wrote:
> value_range_base::operator== was originally lifted from a world where 

> symbolics didn't exist (the ranger branch), but symbolics do exist in 

> mainline.

>

> Although this isn't causing a problem yet, as soon as someone tries to 

> compare non numeric ranges, we'll die.  Using operand_equal_p 

> simplifies the comparison code drastically.

>

> I suppose if/when we get multiple sub-ranges in value_range_base, 

> we'll have to adapt this function again to compare things piece wise.  

> For now, let's keep things simple.

>

> OK pending tests?

Oh, we brought over the multiple sub-range bits to value_range_base... 
yeah we can remove that and just check for operand equality.  we'll deal 
with multiple subranges when thats a thing.


Is this any different than just calling value_range_base::equal_p()?

Andrew
Aldy Hernandez Nov. 4, 2019, 11:05 p.m. | #2
On 11/4/19 11:45 PM, Andrew MacLeod wrote:
> On 11/4/19 5:23 PM, Aldy Hernandez wrote:

>> value_range_base::operator== was originally lifted from a world where 

>> symbolics didn't exist (the ranger branch), but symbolics do exist in 

>> mainline.

>>

>> Although this isn't causing a problem yet, as soon as someone tries to 

>> compare non numeric ranges, we'll die.  Using operand_equal_p 

>> simplifies the comparison code drastically.

>>

>> I suppose if/when we get multiple sub-ranges in value_range_base, 

>> we'll have to adapt this function again to compare things piece wise. 

>> For now, let's keep things simple.

>>

>> OK pending tests?

> Oh, we brought over the multiple sub-range bits to value_range_base... 

> yeah we can remove that and just check for operand equality.  we'll deal 

> with multiple subranges when thats a thing.

> 

> 

> Is this any different than just calling value_range_base::equal_p()?


Ooops, indeed, that's the same thing.

Adjusted.

OK pending tests?
commit eb3ca5b2eeb84233a485981e140c6f910deb4bcc
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Mon Nov 4 21:20:26 2019 +0100

    Use value_range_base::equal_p in value_range_base::operator== so we can handle
    symbolics without dying.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 254b3950a8e..b9f61791270 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2019-11-04  Aldy Hernandez  <aldyh@redhat.com>
+
+	* tree-vrp.c (value_range_base::operator==): Use equal_p to
+	properly handle symbolics.
+
 2019-11-04  Aldy Hernandez  <aldyh@redhat.com>
 
 	* tree-vrp.c (value_range_base::set): Do not special case pointers.
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 452895bfc24..9d6a4ebc7fe 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -6333,19 +6333,7 @@ range_compatible_p (tree t1, tree t2)
 bool
 value_range_base::operator== (const value_range_base &r) const
 {
-  if (undefined_p ())
-    return r.undefined_p ();
-
-  if (num_pairs () != r.num_pairs ()
-      || !range_compatible_p (type (), r.type ()))
-    return false;
-
-  for (unsigned p = 0; p < num_pairs (); p++)
-    if (wi::ne_p (lower_bound (p), r.lower_bound (p))
-	|| wi::ne_p (upper_bound (p), r.upper_bound (p)))
-      return false;
-
-  return true;
+  return equal_p (r);
 }
 
 /* Visit all arguments for PHI node PHI that flow through executable
Andrew MacLeod Nov. 5, 2019, 2:55 a.m. | #3
On 11/4/19 6:05 PM, Aldy Hernandez wrote:
>

>

> On 11/4/19 11:45 PM, Andrew MacLeod wrote:

>> On 11/4/19 5:23 PM, Aldy Hernandez wrote:

>>> value_range_base::operator== was originally lifted from a world 

>>> where symbolics didn't exist (the ranger branch), but symbolics do 

>>> exist in mainline.

>>>

>>> Although this isn't causing a problem yet, as soon as someone tries 

>>> to compare non numeric ranges, we'll die.  Using operand_equal_p 

>>> simplifies the comparison code drastically.

>>>

>>> I suppose if/when we get multiple sub-ranges in value_range_base, 

>>> we'll have to adapt this function again to compare things piece 

>>> wise. For now, let's keep things simple.

>>>

>>> OK pending tests?

>> Oh, we brought over the multiple sub-range bits to 

>> value_range_base... yeah we can remove that and just check for 

>> operand equality.  we'll deal with multiple subranges when thats a 

>> thing.

>>

>>

>> Is this any different than just calling value_range_base::equal_p()?

>

> Ooops, indeed, that's the same thing.

>

> Adjusted.

>

> OK pending tests?

yeah, approved.

Andrew
Aldy Hernandez Nov. 5, 2019, 3:34 a.m. | #4
On 11/5/19 3:55 AM, Andrew MacLeod wrote:
> On 11/4/19 6:05 PM, Aldy Hernandez wrote:

>>

>>

>> On 11/4/19 11:45 PM, Andrew MacLeod wrote:

>>> On 11/4/19 5:23 PM, Aldy Hernandez wrote:

>>>> value_range_base::operator== was originally lifted from a world 

>>>> where symbolics didn't exist (the ranger branch), but symbolics do 

>>>> exist in mainline.

>>>>

>>>> Although this isn't causing a problem yet, as soon as someone tries 

>>>> to compare non numeric ranges, we'll die.  Using operand_equal_p 

>>>> simplifies the comparison code drastically.

>>>>

>>>> I suppose if/when we get multiple sub-ranges in value_range_base, 

>>>> we'll have to adapt this function again to compare things piece 

>>>> wise. For now, let's keep things simple.

>>>>

>>>> OK pending tests?

>>> Oh, we brought over the multiple sub-range bits to 

>>> value_range_base... yeah we can remove that and just check for 

>>> operand equality.  we'll deal with multiple subranges when thats a 

>>> thing.

>>>

>>>

>>> Is this any different than just calling value_range_base::equal_p()?

>>

>> Ooops, indeed, that's the same thing.

>>

>> Adjusted.

>>

>> OK pending tests?

> yeah, approved.


Final committed patch attached.  I had to remove the now unused 
range_compatible_p function, as it's no longer necessary.

Thanks.
commit 10eefa1934bb1833c85d1c445f4da01b933c0909
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Mon Nov 4 21:20:26 2019 +0100

    Use value_range_base::equal_p in value_range_base::operator== so we can handle
    symbolics without dying.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index e726ff6d0a0..1c2fff16295 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,9 @@
+2019-11-05  Aldy Hernandez  <aldyh@redhat.com>
+
+	* tree-vrp.c (value_range_base::operator==): Use equal_p to
+	properly handle symbolics.
+	(range_compatible_p): Remove.
+
 2019-11-04  Kamlesh Kumar  <kamleshbhalui@gmail.com>
 
 	* common.opt (-fabi-version): Document =14.
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 452895bfc24..a6d44e9dc6d 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -6319,33 +6319,10 @@ value_range_base::intersect (const value_range_base &r)
     dump_flags |= TDF_DETAILS;
 }
 
-/* Return TRUE if two types are compatible for range operations.  */
-
-static bool
-range_compatible_p (tree t1, tree t2)
-{
-  if (POINTER_TYPE_P (t1) && POINTER_TYPE_P (t2))
-    return true;
-
-  return types_compatible_p (t1, t2);
-}
-
 bool
 value_range_base::operator== (const value_range_base &r) const
 {
-  if (undefined_p ())
-    return r.undefined_p ();
-
-  if (num_pairs () != r.num_pairs ()
-      || !range_compatible_p (type (), r.type ()))
-    return false;
-
-  for (unsigned p = 0; p < num_pairs (); p++)
-    if (wi::ne_p (lower_bound (p), r.lower_bound (p))
-	|| wi::ne_p (upper_bound (p), r.upper_bound (p)))
-      return false;
-
-  return true;
+  return equal_p (r);
 }
 
 /* Visit all arguments for PHI node PHI that flow through executable

Patch

commit d1177659bd26ac8122410dee092f5ca427b4558b
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Mon Nov 4 21:20:26 2019 +0100

    Use operand_equal_p in value_range_base::operator== so we can handle
    symbolics without dying.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 6fbbf87e294..3ebe7fd4348 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@ 
+2019-11-04  Aldy Hernandez  <aldyh@redhat.com>
+
+	* tree-vrp.c (value_range_base::operator==): Use operand_equal_p
+	instead of wide-int's, to properly handle symbolics.
+
 2019-11-04  Aldy Hernandez  <aldyh@redhat.com>
 
 	* tree-vrp.c (value_range_base::set): Do not special case pointers.
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 452895bfc24..e683339f3cd 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -6336,16 +6336,12 @@  value_range_base::operator== (const value_range_base &r) const
   if (undefined_p ())
     return r.undefined_p ();
 
-  if (num_pairs () != r.num_pairs ()
-      || !range_compatible_p (type (), r.type ()))
+  if (!range_compatible_p (type (), r.type ()))
     return false;
 
-  for (unsigned p = 0; p < num_pairs (); p++)
-    if (wi::ne_p (lower_bound (p), r.lower_bound (p))
-	|| wi::ne_p (upper_bound (p), r.upper_bound (p)))
-      return false;
-
-  return true;
+  return (m_kind == r.m_kind
+	  && operand_equal_p (m_min, r.m_min, 0)
+	  && operand_equal_p (m_max, r.m_max, 0));
 }
 
 /* Visit all arguments for PHI node PHI that flow through executable