Add value_range_base::contains_p

Message ID 6cbe0075-0250-ae99-7a2b-2198cecd40c4@redhat.com
State New
Headers show
Series
  • Add value_range_base::contains_p
Related show

Commit Message

Aldy Hernandez June 11, 2019, 10:40 a.m.
This patch cleans up the various contains, may_contain, and 
value_inside_range variants we have throughout, in favor of one-- 
contains_p.  There should be no changes in functionality.

I have added a note to range_includes_zero_p, perhaps as a personal 
question than anything else.  This function was/is returning true for 
UNDEFINED.  From a semantic sense, that doesn't make sense.  UNDEFINED 
is really the empty set.  Is the functionality wrong, or should we call 
this function something else?  Either way, I'm fine removing the comment 
but I'm genuinely curious.

OK?

Comments

Richard Biener June 11, 2019, 1:45 p.m. | #1
On Tue, Jun 11, 2019 at 12:40 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>

> This patch cleans up the various contains, may_contain, and

> value_inside_range variants we have throughout, in favor of one--

> contains_p.  There should be no changes in functionality.

>

> I have added a note to range_includes_zero_p, perhaps as a personal

> question than anything else.  This function was/is returning true for

> UNDEFINED.  From a semantic sense, that doesn't make sense.  UNDEFINED

> is really the empty set.  Is the functionality wrong, or should we call

> this function something else?  Either way, I'm fine removing the comment

> but I'm genuinely curious.


So this affects for example this hunk:

-      if (vr && !range_includes_p (vr, 1))
+      if (vr && (!vr->contains_p (build_one_cst (TREE_TYPE (name)))
+                && !vr->undefined_p ()))
        {

I think it's arbitrary how we compute X in UNDEFINED and I'm fine
with changing the affected predicates to return false.  This means
not testing for !vr->undefined_p here.

Note I very much dislike the build_one_cst () call here so please
provide an overload hiding this.

Not sure why you keep range_includes_zero_p.

Richard.

>

> OK?
Aldy Hernandez June 11, 2019, 3:05 p.m. | #2
On 6/11/19 9:45 AM, Richard Biener wrote:
> On Tue, Jun 11, 2019 at 12:40 PM Aldy Hernandez <aldyh@redhat.com> wrote:

>>

>> This patch cleans up the various contains, may_contain, and

>> value_inside_range variants we have throughout, in favor of one--

>> contains_p.  There should be no changes in functionality.

>>

>> I have added a note to range_includes_zero_p, perhaps as a personal

>> question than anything else.  This function was/is returning true for

>> UNDEFINED.  From a semantic sense, that doesn't make sense.  UNDEFINED

>> is really the empty set.  Is the functionality wrong, or should we call

>> this function something else?  Either way, I'm fine removing the comment

>> but I'm genuinely curious.

> 

> So this affects for example this hunk:

> 

> -      if (vr && !range_includes_p (vr, 1))

> +      if (vr && (!vr->contains_p (build_one_cst (TREE_TYPE (name)))

> +                && !vr->undefined_p ()))

>          {

> 

> I think it's arbitrary how we compute X in UNDEFINED and I'm fine

> with changing the affected predicates to return false.  This means

> not testing for !vr->undefined_p here.


Excellent.

> 

> Note I very much dislike the build_one_cst () call here so please

> provide an overload hiding this.


Good idea.  I love it.

> 

> Not sure why you keep range_includes_zero_p.


I wasn't sure if there was some subtle reason why we were including 
undefined.

OK pending tests?
commit 67ce79ed3cc0729650873b1c409d9a4c5156aad9
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Tue Jun 4 09:11:22 2019 +0200

    value_range_base::contains_p and may_contains_p.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index b9f032e2de3..df3b9abc126 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,27 @@
+2019-06-11  Aldy Hernandez  <aldyh@redhat.com>
+
+	* gimple-loop-versioning.cc (prune_loop_conditions): Use
+	value_range_base::contains_p.
+	* gimple-ssa-evrp-analyze.c (set_ssa_range_info): Use
+	value_range_base::contains_p.
+	* tree-vrp.c (value_range_base::may_contain_p): Remove.
+	(value_inside_range): Make private to value_range_base class.
+	(value_range_base::contains_p): New.
+	(extract_range_from_binary_expr): Use contains_p.
+	(extract_range_from_unary_expr): Same.
+	(union_helper): Same.
+	(vrp_prop::vrp_finalize): Same.
+	* tree-vrp.h (value_range_base): Remove may_contain_p.  Add
+	contains_p.  Add value_inside_range.
+	(range_includes_p): Remove.
+	(value_inside_range): Remove.
+	(range_includes_zero_p): Remove.
+	(range_includes_p): Use value_range_base::contains_p.
+	* vr-values.c (compare_range_with_value): Same.
+	(vrp_stmt_computes_nonzero): Use value_range_base::contains_p.
+	(extract_range_basic): Same.
+	(compare_range_with_value): Same.
+
 2019-06-11  Aldy Hernandez  <aldyh@redhat.com>
 
 	* gimple-ssa-evrp.c (evrp_dom_walker::before_dom_children): Use
diff --git a/gcc/gimple-loop-versioning.cc b/gcc/gimple-loop-versioning.cc
index 2f7cda9c8cb..41c2ae33d68 100644
--- a/gcc/gimple-loop-versioning.cc
+++ b/gcc/gimple-loop-versioning.cc
@@ -1488,7 +1488,7 @@ loop_versioning::prune_loop_conditions (struct loop *loop, vr_values *vrs)
     {
       tree name = ssa_name (i);
       value_range *vr = vrs->get_value_range (name);
-      if (vr && !range_includes_p (vr, 1))
+      if (vr && !vr->contains_p (1))
 	{
 	  if (dump_enabled_p ())
 	    dump_printf_loc (MSG_NOTE, find_loop_location (loop),
diff --git a/gcc/gimple-ssa-evrp-analyze.c b/gcc/gimple-ssa-evrp-analyze.c
index bb4e2d6e798..92e2325e9f5 100644
--- a/gcc/gimple-ssa-evrp-analyze.c
+++ b/gcc/gimple-ssa-evrp-analyze.c
@@ -118,8 +118,7 @@ evrp_range_analyzer::set_ssa_range_info (tree lhs, value_range *vr)
 			wi::to_wide (vr->min ()),
 			wi::to_wide (vr->max ()));
     }
-  else if (POINTER_TYPE_P (TREE_TYPE (lhs))
-	   && range_includes_zero_p (vr) == 0)
+  else if (POINTER_TYPE_P (TREE_TYPE (lhs)) && !vr->contains_p (0))
     set_ptr_nonnull (lhs);
 }
 
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index e5000dbcd08..945ecdf250e 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -282,25 +282,6 @@ value_range::set_varying ()
   set (VR_VARYING, NULL, NULL, NULL);
 }
 
-/* Return TRUE if it is possible that range contains VAL.  */
-
-bool
-value_range_base::may_contain_p (tree val) const
-{
-  if (varying_p ())
-    return true;
-
-  if (undefined_p ())
-    return true;
-
-  if (m_kind == VR_ANTI_RANGE)
-    {
-      int res = value_inside_range (val, min (), max ());
-      return res == 0 || res == -2;
-    }
-  return value_inside_range (val, min (), max ()) != 0;
-}
-
 void
 value_range::equiv_clear ()
 {
@@ -1126,7 +1107,7 @@ compare_values (tree val1, tree val2)
    function.  */
 
 int
-value_inside_range (tree val, tree min, tree max)
+value_range_base::value_inside_range (tree val, tree min, tree max) const
 {
   int cmp1, cmp2;
 
@@ -1143,15 +1124,37 @@ value_inside_range (tree val, tree min, tree max)
   return !cmp2;
 }
 
+/* Return TRUE if range contains VAL.  */
+
+bool
+value_range_base::contains_p (tree val) const
+{
+  if (varying_p ())
+    return true;
+
+  if (undefined_p ())
+    return false;
 
-/* Return TRUE if *VR includes the value X.  */
+  if (m_kind == VR_ANTI_RANGE)
+    {
+      int res = value_inside_range (val, min (), max ());
+      return res == 0 || res == -2;
+    }
+  return value_inside_range (val, min (), max ()) != 0;
+}
+
+/* Return TRUE if range contains VAL.  */
 
 bool
-range_includes_p (const value_range_base *vr, HOST_WIDE_INT x)
+value_range_base::contains_p (int val) const
 {
-  if (vr->varying_p () || vr->undefined_p ())
+  if (varying_p ())
     return true;
-  return vr->may_contain_p (build_int_cst (vr->type (), x));
+
+  if (undefined_p ())
+    return false;
+
+  return contains_p (build_int_cst (type (), val));
 }
 
 /* Value range wrapper for wide_int_range_set_zero_nonzero_bits.
@@ -1618,7 +1621,7 @@ extract_range_from_binary_expr (value_range_base *vr,
 	     nullness, if both are non null, then the result is nonnull.
 	     If both are null, then the result is null. Otherwise they
 	     are varying.  */
-	  if (!range_includes_zero_p (&vr0) && !range_includes_zero_p (&vr1))
+	  if (!vr0.contains_p (0) && !vr1.contains_p (0))
 	    vr->set_nonzero (expr_type);
 	  else if (vr0.zero_p () && vr1.zero_p ())
 	    vr->set_zero (expr_type);
@@ -1642,8 +1645,7 @@ extract_range_from_binary_expr (value_range_base *vr,
 	     doesn't either.  As the second operand is sizetype (unsigned),
 	     consider all ranges where the MSB could be set as possible
 	     subtractions where the result might be NULL.  */
-	  if ((!range_includes_zero_p (&vr0)
-	       || !range_includes_zero_p (&vr1))
+	  if ((!vr0.contains_p (0) || !vr1.contains_p (0))
 	      && !TYPE_OVERFLOW_WRAPS (expr_type)
 	      && (flag_delete_null_pointer_checks
 		  || (range_int_cst_p (&vr1)
@@ -1658,7 +1660,7 @@ extract_range_from_binary_expr (value_range_base *vr,
 	{
 	  /* For pointer types, we are really only interested in asserting
 	     whether the expression evaluates to non-NULL.  */
-	  if (!range_includes_zero_p (&vr0) && !range_includes_zero_p (&vr1))
+	  if (!vr0.contains_p (0) && !vr1.contains_p (0))
 	    vr->set_nonzero (expr_type);
 	  else if (vr0.zero_p () || vr1.zero_p ())
 	    vr->set_zero (expr_type);
@@ -2096,7 +2098,7 @@ extract_range_from_unary_expr (value_range_base *vr,
 	 storing a magic constant to a pointer.  */
       if (POINTER_TYPE_P (type) || POINTER_TYPE_P (op0_type))
 	{
-	  if (!range_includes_zero_p (&vr0))
+	  if (!vr0.contains_p (0))
 	    vr->set_nonzero (type);
 	  else if (vr0.zero_p ())
 	    vr->set_zero (type);
@@ -6114,9 +6116,7 @@ value_range_base::union_helper (const value_range_base *vr0,
   /* Failed to find an efficient meet.  Before giving up and setting
      the result to VARYING, see if we can at least derive a useful
      anti-range.  */
-  if (tem.varying_p ()
-      && range_includes_zero_p (vr0) == 0
-      && range_includes_zero_p (vr1) == 0)
+  if (tem.varying_p () && !vr0->contains_p (0) && !vr1->contains_p (0))
     {
       tem.set_nonzero (vr0->type ());
       return tem;
@@ -6639,8 +6639,7 @@ vrp_prop::vrp_finalize (bool warn_array_bounds_p)
       if (!name || !vr->constant_p ())
 	continue;
 
-      if (POINTER_TYPE_P (TREE_TYPE (name))
-	  && range_includes_zero_p (vr) == 0)
+      if (POINTER_TYPE_P (TREE_TYPE (name)) && !vr->contains_p (0))
 	set_ptr_nonnull (name);
       else if (!POINTER_TYPE_P (TREE_TYPE (name)))
 	set_range_info (name, *vr);
diff --git a/gcc/tree-vrp.h b/gcc/tree-vrp.h
index 1c4af7b68fb..5100e88b95d 100644
--- a/gcc/tree-vrp.h
+++ b/gcc/tree-vrp.h
@@ -69,7 +69,8 @@ public:
 
   /* Misc methods.  */
   tree type () const;
-  bool may_contain_p (tree) const;
+  bool contains_p (tree) const;
+  bool contains_p (int) const;
   void set_and_canonicalize (enum value_range_kind, tree, tree);
   bool zero_p () const;
   bool nonzero_p () const;
@@ -94,6 +95,9 @@ protected:
   friend void gt_ggc_mx (value_range_base *&);
   friend void gt_pch_nx (value_range_base &);
   friend void gt_pch_nx (value_range_base *, gt_pointer_operator, void *);
+
+private:
+  int value_inside_range (tree, tree, tree) const;
 };
 
 /* Note value_range cannot currently be used with GC memory, only
@@ -252,7 +256,6 @@ struct assert_info
 extern void register_edge_assert_for (tree, edge, enum tree_code,
 				      tree, tree, vec<assert_info> &);
 extern bool stmt_interesting_for_vrp (gimple *);
-extern bool range_includes_p (const value_range_base *, HOST_WIDE_INT);
 extern bool infer_value_range (gimple *, tree, tree_code *, tree *);
 
 extern bool vrp_bitmap_equal_p (const_bitmap, const_bitmap);
@@ -265,7 +268,6 @@ extern int compare_values_warnv (tree, tree, bool *);
 extern int operand_less_p (tree, tree);
 extern bool vrp_val_is_min (const_tree);
 extern bool vrp_val_is_max (const_tree);
-extern int value_inside_range (tree, tree, tree);
 
 extern tree vrp_val_min (const_tree);
 extern tree vrp_val_max (const_tree);
@@ -293,12 +295,4 @@ extern tree get_single_symbol (tree, bool *, tree *);
 extern void maybe_set_nonzero_bits (edge, tree);
 extern value_range_kind determine_value_range (tree, wide_int *, wide_int *);
 
-/* Return TRUE if *VR includes the value zero.  */
-
-inline bool
-range_includes_zero_p (const value_range_base *vr)
-{
-  return range_includes_p (vr, 0);
-}
-
 #endif /* GCC_TREE_VRP_H */
diff --git a/gcc/vr-values.c b/gcc/vr-values.c
index 97235a56c50..bf7666a052b 100644
--- a/gcc/vr-values.c
+++ b/gcc/vr-values.c
@@ -340,7 +340,7 @@ vr_values::vrp_stmt_computes_nonzero (gimple *stmt)
 		  && !TYPE_OVERFLOW_WRAPS (TREE_TYPE (expr))))
 	    {
 	      value_range *vr = get_value_range (TREE_OPERAND (base, 0));
-	      if (!range_includes_zero_p (vr))
+	      if (!vr->contains_p (0))
 		return true;
 	    }
 	  /* If MEM_REF has a "positive" offset, consider it non-NULL
@@ -1115,7 +1115,7 @@ vr_values::extract_range_basic (value_range *vr, gimple *stmt)
 	    {
 	      value_range *vr0 = get_value_range (arg);
 	      /* If arg is non-zero, then ffs or popcount are non-zero.  */
-	      if (range_includes_zero_p (vr0) == 0)
+	      if (!vr0->contains_p (0))
 		mini = 1;
 	      /* If some high bits are known to be zero,
 		 we can decrease the maximum.  */
@@ -1624,8 +1624,7 @@ compare_range_with_value (enum tree_code comp, value_range *vr, tree val,
 	  || comp == LE_EXPR)
 	return NULL_TREE;
 
-      /* ~[VAL_1, VAL_2] OP VAL is known if VAL_1 <= VAL <= VAL_2.  */
-      if (value_inside_range (val, vr->min (), vr->max ()) == 1)
+      if (!vr->contains_p (val))
 	return (comp == NE_EXPR) ? boolean_true_node : boolean_false_node;
 
       return NULL_TREE;
Martin Sebor June 11, 2019, 4:17 p.m. | #3
On 6/11/19 9:05 AM, Aldy Hernandez wrote:
> 

> 

> On 6/11/19 9:45 AM, Richard Biener wrote:

>> On Tue, Jun 11, 2019 at 12:40 PM Aldy Hernandez <aldyh@redhat.com> wrote:

>>>

>>> This patch cleans up the various contains, may_contain, and

>>> value_inside_range variants we have throughout, in favor of one--

>>> contains_p.  There should be no changes in functionality.

>>>

>>> I have added a note to range_includes_zero_p, perhaps as a personal

>>> question than anything else.  This function was/is returning true for

>>> UNDEFINED.  From a semantic sense, that doesn't make sense.  UNDEFINED

>>> is really the empty set.  Is the functionality wrong, or should we call

>>> this function something else?  Either way, I'm fine removing the comment

>>> but I'm genuinely curious.

>>

>> So this affects for example this hunk:

>>

>> -      if (vr && !range_includes_p (vr, 1))

>> +      if (vr && (!vr->contains_p (build_one_cst (TREE_TYPE (name)))

>> +                && !vr->undefined_p ()))

>>          {

>>

>> I think it's arbitrary how we compute X in UNDEFINED and I'm fine

>> with changing the affected predicates to return false.  This means

>> not testing for !vr->undefined_p here.

> 

> Excellent.

> 

>>

>> Note I very much dislike the build_one_cst () call here so please

>> provide an overload hiding this.

> 

> Good idea.  I love it.

> 

>>

>> Not sure why you keep range_includes_zero_p.

> 

> I wasn't sure if there was some subtle reason why we were including 

> undefined.

> 

> OK pending tests?


Should the second overload:

+  bool contains_p (tree) const;
+  bool contains_p (int) const;

take something like HOST_WIDE_INT or even one of those poly_ints
like build_int_cst does?  (With the former, contains_p (0) will
likely be ambiguous since 0 is int and HOST_WIDE_INT is long).

Martin
Aldy Hernandez June 11, 2019, 4:26 p.m. | #4
On 6/11/19 12:17 PM, Martin Sebor wrote:
> On 6/11/19 9:05 AM, Aldy Hernandez wrote:

>>

>>

>> On 6/11/19 9:45 AM, Richard Biener wrote:

>>> On Tue, Jun 11, 2019 at 12:40 PM Aldy Hernandez <aldyh@redhat.com> 

>>> wrote:

>>>>

>>>> This patch cleans up the various contains, may_contain, and

>>>> value_inside_range variants we have throughout, in favor of one--

>>>> contains_p.  There should be no changes in functionality.

>>>>

>>>> I have added a note to range_includes_zero_p, perhaps as a personal

>>>> question than anything else.  This function was/is returning true for

>>>> UNDEFINED.  From a semantic sense, that doesn't make sense.  UNDEFINED

>>>> is really the empty set.  Is the functionality wrong, or should we call

>>>> this function something else?  Either way, I'm fine removing the 

>>>> comment

>>>> but I'm genuinely curious.

>>>

>>> So this affects for example this hunk:

>>>

>>> -      if (vr && !range_includes_p (vr, 1))

>>> +      if (vr && (!vr->contains_p (build_one_cst (TREE_TYPE (name)))

>>> +                && !vr->undefined_p ()))

>>>          {

>>>

>>> I think it's arbitrary how we compute X in UNDEFINED and I'm fine

>>> with changing the affected predicates to return false.  This means

>>> not testing for !vr->undefined_p here.

>>

>> Excellent.

>>

>>>

>>> Note I very much dislike the build_one_cst () call here so please

>>> provide an overload hiding this.

>>

>> Good idea.  I love it.

>>

>>>

>>> Not sure why you keep range_includes_zero_p.

>>

>> I wasn't sure if there was some subtle reason why we were including 

>> undefined.

>>

>> OK pending tests?

> 

> Should the second overload:

> 

> +  bool contains_p (tree) const;

> +  bool contains_p (int) const;

> 

> take something like HOST_WIDE_INT or even one of those poly_ints

> like build_int_cst does?  (With the former, contains_p (0) will

> likely be ambiguous since 0 is int and HOST_WIDE_INT is long).


We have a type, so there should be no confusion:

+  return contains_p (build_int_cst (type (), val));

(UNDEFINED and VARYING don't have a type, so they are special cased prior).

Aldy
Martin Sebor June 11, 2019, 4:52 p.m. | #5
On 6/11/19 10:26 AM, Aldy Hernandez wrote:
> 

> 

> On 6/11/19 12:17 PM, Martin Sebor wrote:

>> On 6/11/19 9:05 AM, Aldy Hernandez wrote:

>>>

>>>

>>> On 6/11/19 9:45 AM, Richard Biener wrote:

>>>> On Tue, Jun 11, 2019 at 12:40 PM Aldy Hernandez <aldyh@redhat.com> 

>>>> wrote:

>>>>>

>>>>> This patch cleans up the various contains, may_contain, and

>>>>> value_inside_range variants we have throughout, in favor of one--

>>>>> contains_p.  There should be no changes in functionality.

>>>>>

>>>>> I have added a note to range_includes_zero_p, perhaps as a personal

>>>>> question than anything else.  This function was/is returning true for

>>>>> UNDEFINED.  From a semantic sense, that doesn't make sense.  UNDEFINED

>>>>> is really the empty set.  Is the functionality wrong, or should we 

>>>>> call

>>>>> this function something else?  Either way, I'm fine removing the 

>>>>> comment

>>>>> but I'm genuinely curious.

>>>>

>>>> So this affects for example this hunk:

>>>>

>>>> -      if (vr && !range_includes_p (vr, 1))

>>>> +      if (vr && (!vr->contains_p (build_one_cst (TREE_TYPE (name)))

>>>> +                && !vr->undefined_p ()))

>>>>          {

>>>>

>>>> I think it's arbitrary how we compute X in UNDEFINED and I'm fine

>>>> with changing the affected predicates to return false.  This means

>>>> not testing for !vr->undefined_p here.

>>>

>>> Excellent.

>>>

>>>>

>>>> Note I very much dislike the build_one_cst () call here so please

>>>> provide an overload hiding this.

>>>

>>> Good idea.  I love it.

>>>

>>>>

>>>> Not sure why you keep range_includes_zero_p.

>>>

>>> I wasn't sure if there was some subtle reason why we were including 

>>> undefined.

>>>

>>> OK pending tests?

>>

>> Should the second overload:

>>

>> +  bool contains_p (tree) const;

>> +  bool contains_p (int) const;

>>

>> take something like HOST_WIDE_INT or even one of those poly_ints

>> like build_int_cst does?  (With the former, contains_p (0) will

>> likely be ambiguous since 0 is int and HOST_WIDE_INT is long).

> 

> We have a type, so there should be no confusion:

> 

> +  return contains_p (build_int_cst (type (), val));

> 

> (UNDEFINED and VARYING don't have a type, so they are special cased prior).


I didn't mean the overloads are confusing, just that there the one
that takes an int doesn't make it possible to test whether a value
outside the range of an int is in the range.  For example, in
the call

   contains_p (SIZE_MAX)

the argument will get sliced (and trigger a -Woverflow).  One will
need to go back to the more verbose way of calling it.

Changing the argument type to HOST_WIDE_INT would avoid the slicing
and warning but then make contains_p (0) ambiguous because 0 isn't
a perfect match for either void* or long (so it requires a conversion).

Martin

> 

> Aldy
Aldy Hernandez June 11, 2019, 9:02 p.m. | #6
On 6/11/19 12:52 PM, Martin Sebor wrote:
> On 6/11/19 10:26 AM, Aldy Hernandez wrote:

>>

>>

>> On 6/11/19 12:17 PM, Martin Sebor wrote:

>>> On 6/11/19 9:05 AM, Aldy Hernandez wrote:

>>>>

>>>>

>>>> On 6/11/19 9:45 AM, Richard Biener wrote:

>>>>> On Tue, Jun 11, 2019 at 12:40 PM Aldy Hernandez <aldyh@redhat.com> 

>>>>> wrote:

>>>>>>

>>>>>> This patch cleans up the various contains, may_contain, and

>>>>>> value_inside_range variants we have throughout, in favor of one--

>>>>>> contains_p.  There should be no changes in functionality.

>>>>>>

>>>>>> I have added a note to range_includes_zero_p, perhaps as a personal

>>>>>> question than anything else.  This function was/is returning true for

>>>>>> UNDEFINED.  From a semantic sense, that doesn't make sense.  

>>>>>> UNDEFINED

>>>>>> is really the empty set.  Is the functionality wrong, or should we 

>>>>>> call

>>>>>> this function something else?  Either way, I'm fine removing the 

>>>>>> comment

>>>>>> but I'm genuinely curious.

>>>>>

>>>>> So this affects for example this hunk:

>>>>>

>>>>> -      if (vr && !range_includes_p (vr, 1))

>>>>> +      if (vr && (!vr->contains_p (build_one_cst (TREE_TYPE (name)))

>>>>> +                && !vr->undefined_p ()))

>>>>>          {

>>>>>

>>>>> I think it's arbitrary how we compute X in UNDEFINED and I'm fine

>>>>> with changing the affected predicates to return false.  This means

>>>>> not testing for !vr->undefined_p here.

>>>>

>>>> Excellent.

>>>>

>>>>>

>>>>> Note I very much dislike the build_one_cst () call here so please

>>>>> provide an overload hiding this.

>>>>

>>>> Good idea.  I love it.

>>>>

>>>>>

>>>>> Not sure why you keep range_includes_zero_p.

>>>>

>>>> I wasn't sure if there was some subtle reason why we were including 

>>>> undefined.

>>>>

>>>> OK pending tests?

>>>

>>> Should the second overload:

>>>

>>> +  bool contains_p (tree) const;

>>> +  bool contains_p (int) const;

>>>

>>> take something like HOST_WIDE_INT or even one of those poly_ints

>>> like build_int_cst does?  (With the former, contains_p (0) will

>>> likely be ambiguous since 0 is int and HOST_WIDE_INT is long).

>>

>> We have a type, so there should be no confusion:

>>

>> +  return contains_p (build_int_cst (type (), val));

>>

>> (UNDEFINED and VARYING don't have a type, so they are special cased 

>> prior).

> 

> I didn't mean the overloads are confusing, just that there the one

> that takes an int doesn't make it possible to test whether a value

> outside the range of an int is in the range.  For example, in

> the call

> 

>    contains_p (SIZE_MAX)

> 

> the argument will get sliced (and trigger a -Woverflow).  One will

> need to go back to the more verbose way of calling it.


The int version is not really meant to pass anything but simple 
constants.  For anything fancy, one should really be using the tree 
version.  But I can certainly change the argument to HOST_WIDE_INT if 
preferred.

> 

> Changing the argument type to HOST_WIDE_INT would avoid the slicing

> and warning but then make contains_p (0) ambiguous because 0 isn't

> a perfect match for either void* or long (so it requires a conversion).


Just a plain 0 will match the int version, instead of the tree version, 
right?  Nobody should be passing NULL to the tree version, so that seems 
like a non-issue.

Aldy
Martin Sebor June 11, 2019, 11:57 p.m. | #7
On 6/11/19 3:02 PM, Aldy Hernandez wrote:
> 

> 

> On 6/11/19 12:52 PM, Martin Sebor wrote:

>> On 6/11/19 10:26 AM, Aldy Hernandez wrote:

>>>

>>>

>>> On 6/11/19 12:17 PM, Martin Sebor wrote:

>>>> On 6/11/19 9:05 AM, Aldy Hernandez wrote:

>>>>>

>>>>>

>>>>> On 6/11/19 9:45 AM, Richard Biener wrote:

>>>>>> On Tue, Jun 11, 2019 at 12:40 PM Aldy Hernandez <aldyh@redhat.com> 

>>>>>> wrote:

>>>>>>>

>>>>>>> This patch cleans up the various contains, may_contain, and

>>>>>>> value_inside_range variants we have throughout, in favor of one--

>>>>>>> contains_p.  There should be no changes in functionality.

>>>>>>>

>>>>>>> I have added a note to range_includes_zero_p, perhaps as a personal

>>>>>>> question than anything else.  This function was/is returning true 

>>>>>>> for

>>>>>>> UNDEFINED.  From a semantic sense, that doesn't make sense. 

>>>>>>> UNDEFINED

>>>>>>> is really the empty set.  Is the functionality wrong, or should 

>>>>>>> we call

>>>>>>> this function something else?  Either way, I'm fine removing the 

>>>>>>> comment

>>>>>>> but I'm genuinely curious.

>>>>>>

>>>>>> So this affects for example this hunk:

>>>>>>

>>>>>> -      if (vr && !range_includes_p (vr, 1))

>>>>>> +      if (vr && (!vr->contains_p (build_one_cst (TREE_TYPE (name)))

>>>>>> +                && !vr->undefined_p ()))

>>>>>>          {

>>>>>>

>>>>>> I think it's arbitrary how we compute X in UNDEFINED and I'm fine

>>>>>> with changing the affected predicates to return false.  This means

>>>>>> not testing for !vr->undefined_p here.

>>>>>

>>>>> Excellent.

>>>>>

>>>>>>

>>>>>> Note I very much dislike the build_one_cst () call here so please

>>>>>> provide an overload hiding this.

>>>>>

>>>>> Good idea.  I love it.

>>>>>

>>>>>>

>>>>>> Not sure why you keep range_includes_zero_p.

>>>>>

>>>>> I wasn't sure if there was some subtle reason why we were including 

>>>>> undefined.

>>>>>

>>>>> OK pending tests?

>>>>

>>>> Should the second overload:

>>>>

>>>> +  bool contains_p (tree) const;

>>>> +  bool contains_p (int) const;

>>>>

>>>> take something like HOST_WIDE_INT or even one of those poly_ints

>>>> like build_int_cst does?  (With the former, contains_p (0) will

>>>> likely be ambiguous since 0 is int and HOST_WIDE_INT is long).

>>>

>>> We have a type, so there should be no confusion:

>>>

>>> +  return contains_p (build_int_cst (type (), val));

>>>

>>> (UNDEFINED and VARYING don't have a type, so they are special cased 

>>> prior).

>>

>> I didn't mean the overloads are confusing, just that there the one

>> that takes an int doesn't make it possible to test whether a value

>> outside the range of an int is in the range.  For example, in

>> the call

>>

>>    contains_p (SIZE_MAX)

>>

>> the argument will get sliced (and trigger a -Woverflow).  One will

>> need to go back to the more verbose way of calling it.

> 

> The int version is not really meant to pass anything but simple 

> constants.  For anything fancy, one should really be using the tree 

> version.  But I can certainly change the argument to HOST_WIDE_INT if 

> preferred.

> 

>>

>> Changing the argument type to HOST_WIDE_INT would avoid the slicing

>> and warning but then make contains_p (0) ambiguous because 0 isn't

>> a perfect match for either void* or long (so it requires a conversion).

> 

> Just a plain 0 will match the int version, instead of the tree version, 

> right?  Nobody should be passing NULL to the tree version, so that seems 

> like a non-issue.


Right, NULL isn't a problem, but I would expect any integer to work
(I thought that's what Richard was asking for)  So my suggestion was
to have contains_p() a poly_int64 and provide just as robust an API
as build_int_cst.  The argument ends up converted to the poly_int64
anyway when it's passed to the latter.  I.e., why not define
contains_p simply like this:

   bool
   value_range_base::contains_p (poly_int64 val) const
   {
     if (varying_p ())
       return true;
     if (undefined_p ())
       return false;

     return contains_p (build_int_cst (type (), val));
   }

Martin
Richard Biener June 12, 2019, 9:33 a.m. | #8
On Wed, Jun 12, 2019 at 1:57 AM Martin Sebor <msebor@gmail.com> wrote:
>

> On 6/11/19 3:02 PM, Aldy Hernandez wrote:

> >

> >

> > On 6/11/19 12:52 PM, Martin Sebor wrote:

> >> On 6/11/19 10:26 AM, Aldy Hernandez wrote:

> >>>

> >>>

> >>> On 6/11/19 12:17 PM, Martin Sebor wrote:

> >>>> On 6/11/19 9:05 AM, Aldy Hernandez wrote:

> >>>>>

> >>>>>

> >>>>> On 6/11/19 9:45 AM, Richard Biener wrote:

> >>>>>> On Tue, Jun 11, 2019 at 12:40 PM Aldy Hernandez <aldyh@redhat.com>

> >>>>>> wrote:

> >>>>>>>

> >>>>>>> This patch cleans up the various contains, may_contain, and

> >>>>>>> value_inside_range variants we have throughout, in favor of one--

> >>>>>>> contains_p.  There should be no changes in functionality.

> >>>>>>>

> >>>>>>> I have added a note to range_includes_zero_p, perhaps as a personal

> >>>>>>> question than anything else.  This function was/is returning true

> >>>>>>> for

> >>>>>>> UNDEFINED.  From a semantic sense, that doesn't make sense.

> >>>>>>> UNDEFINED

> >>>>>>> is really the empty set.  Is the functionality wrong, or should

> >>>>>>> we call

> >>>>>>> this function something else?  Either way, I'm fine removing the

> >>>>>>> comment

> >>>>>>> but I'm genuinely curious.

> >>>>>>

> >>>>>> So this affects for example this hunk:

> >>>>>>

> >>>>>> -      if (vr && !range_includes_p (vr, 1))

> >>>>>> +      if (vr && (!vr->contains_p (build_one_cst (TREE_TYPE (name)))

> >>>>>> +                && !vr->undefined_p ()))

> >>>>>>          {

> >>>>>>

> >>>>>> I think it's arbitrary how we compute X in UNDEFINED and I'm fine

> >>>>>> with changing the affected predicates to return false.  This means

> >>>>>> not testing for !vr->undefined_p here.

> >>>>>

> >>>>> Excellent.

> >>>>>

> >>>>>>

> >>>>>> Note I very much dislike the build_one_cst () call here so please

> >>>>>> provide an overload hiding this.

> >>>>>

> >>>>> Good idea.  I love it.

> >>>>>

> >>>>>>

> >>>>>> Not sure why you keep range_includes_zero_p.

> >>>>>

> >>>>> I wasn't sure if there was some subtle reason why we were including

> >>>>> undefined.

> >>>>>

> >>>>> OK pending tests?

> >>>>

> >>>> Should the second overload:

> >>>>

> >>>> +  bool contains_p (tree) const;

> >>>> +  bool contains_p (int) const;

> >>>>

> >>>> take something like HOST_WIDE_INT or even one of those poly_ints

> >>>> like build_int_cst does?  (With the former, contains_p (0) will

> >>>> likely be ambiguous since 0 is int and HOST_WIDE_INT is long).

> >>>

> >>> We have a type, so there should be no confusion:

> >>>

> >>> +  return contains_p (build_int_cst (type (), val));

> >>>

> >>> (UNDEFINED and VARYING don't have a type, so they are special cased

> >>> prior).

> >>

> >> I didn't mean the overloads are confusing, just that there the one

> >> that takes an int doesn't make it possible to test whether a value

> >> outside the range of an int is in the range.  For example, in

> >> the call

> >>

> >>    contains_p (SIZE_MAX)

> >>

> >> the argument will get sliced (and trigger a -Woverflow).  One will

> >> need to go back to the more verbose way of calling it.

> >

> > The int version is not really meant to pass anything but simple

> > constants.  For anything fancy, one should really be using the tree

> > version.  But I can certainly change the argument to HOST_WIDE_INT if

> > preferred.

> >

> >>

> >> Changing the argument type to HOST_WIDE_INT would avoid the slicing

> >> and warning but then make contains_p (0) ambiguous because 0 isn't

> >> a perfect match for either void* or long (so it requires a conversion).

> >

> > Just a plain 0 will match the int version, instead of the tree version,

> > right?  Nobody should be passing NULL to the tree version, so that seems

> > like a non-issue.

>

> Right, NULL isn't a problem, but I would expect any integer to work

> (I thought that's what Richard was asking for)  So my suggestion was

> to have contains_p() a poly_int64 and provide just as robust an API

> as build_int_cst.  The argument ends up converted to the poly_int64

> anyway when it's passed to the latter.  I.e., why not define

> contains_p simply like this:

>

>    bool

>    value_range_base::contains_p (poly_int64 val) const

>    {

>      if (varying_p ())

>        return true;

>      if (undefined_p ())

>        return false;

>

>      return contains_p (build_int_cst (type (), val));

>    }


I agree that plain 'int' is bad.  Given we currently store
INTEGER_CSTs only (and not POLY_INT_CSTs) a
widest_int argument should be fine.  Note widest
because when interpreted signed all unsigned values
fit.

The existing contains_p check is also a bit fishy
for the cases TREE_TYPE of the value has a
value-range not covered in the value-ranges type...
I guess it could do

   if (!int_fits_type_p (val, this->type ())
     return false;

but that changes existing behavior which happily
throws different sign constants into tree_int_cst_lt
for example...  Do we want to allow non-INTEGER_CST
val arguments at all?  That is, you make contains_p
return a bool and say "yes" when we couldn't really
decide.  This is why previously it was named
may_contain_p (and we didn't have must_contain_p).
I think making the result explicitely clear is a must
since contains_p reads like !contains_p means
it does _not_ contain.  So, either change back the
name (and provide the must variant)
or make it return the tri-state from value_inside_range.

Richard.

> Martin
Aldy Hernandez June 12, 2019, 9:33 p.m. | #9
On 6/12/19 5:33 AM, Richard Biener wrote:
> On Wed, Jun 12, 2019 at 1:57 AM Martin Sebor <msebor@gmail.com> wrote:

>>

>> On 6/11/19 3:02 PM, Aldy Hernandez wrote:

>>>

>>>

>>> On 6/11/19 12:52 PM, Martin Sebor wrote:

>>>> On 6/11/19 10:26 AM, Aldy Hernandez wrote:

>>>>>

>>>>>

>>>>> On 6/11/19 12:17 PM, Martin Sebor wrote:

>>>>>> On 6/11/19 9:05 AM, Aldy Hernandez wrote:

>>>>>>>

>>>>>>>

>>>>>>> On 6/11/19 9:45 AM, Richard Biener wrote:

>>>>>>>> On Tue, Jun 11, 2019 at 12:40 PM Aldy Hernandez <aldyh@redhat.com>

>>>>>>>> wrote:

>>>>>>>>>

>>>>>>>>> This patch cleans up the various contains, may_contain, and

>>>>>>>>> value_inside_range variants we have throughout, in favor of one--

>>>>>>>>> contains_p.  There should be no changes in functionality.

>>>>>>>>>

>>>>>>>>> I have added a note to range_includes_zero_p, perhaps as a personal

>>>>>>>>> question than anything else.  This function was/is returning true

>>>>>>>>> for

>>>>>>>>> UNDEFINED.  From a semantic sense, that doesn't make sense.

>>>>>>>>> UNDEFINED

>>>>>>>>> is really the empty set.  Is the functionality wrong, or should

>>>>>>>>> we call

>>>>>>>>> this function something else?  Either way, I'm fine removing the

>>>>>>>>> comment

>>>>>>>>> but I'm genuinely curious.

>>>>>>>>

>>>>>>>> So this affects for example this hunk:

>>>>>>>>

>>>>>>>> -      if (vr && !range_includes_p (vr, 1))

>>>>>>>> +      if (vr && (!vr->contains_p (build_one_cst (TREE_TYPE (name)))

>>>>>>>> +                && !vr->undefined_p ()))

>>>>>>>>           {

>>>>>>>>

>>>>>>>> I think it's arbitrary how we compute X in UNDEFINED and I'm fine

>>>>>>>> with changing the affected predicates to return false.  This means

>>>>>>>> not testing for !vr->undefined_p here.

>>>>>>>

>>>>>>> Excellent.

>>>>>>>

>>>>>>>>

>>>>>>>> Note I very much dislike the build_one_cst () call here so please

>>>>>>>> provide an overload hiding this.

>>>>>>>

>>>>>>> Good idea.  I love it.

>>>>>>>

>>>>>>>>

>>>>>>>> Not sure why you keep range_includes_zero_p.

>>>>>>>

>>>>>>> I wasn't sure if there was some subtle reason why we were including

>>>>>>> undefined.

>>>>>>>

>>>>>>> OK pending tests?

>>>>>>

>>>>>> Should the second overload:

>>>>>>

>>>>>> +  bool contains_p (tree) const;

>>>>>> +  bool contains_p (int) const;

>>>>>>

>>>>>> take something like HOST_WIDE_INT or even one of those poly_ints

>>>>>> like build_int_cst does?  (With the former, contains_p (0) will

>>>>>> likely be ambiguous since 0 is int and HOST_WIDE_INT is long).

>>>>>

>>>>> We have a type, so there should be no confusion:

>>>>>

>>>>> +  return contains_p (build_int_cst (type (), val));

>>>>>

>>>>> (UNDEFINED and VARYING don't have a type, so they are special cased

>>>>> prior).

>>>>

>>>> I didn't mean the overloads are confusing, just that there the one

>>>> that takes an int doesn't make it possible to test whether a value

>>>> outside the range of an int is in the range.  For example, in

>>>> the call

>>>>

>>>>     contains_p (SIZE_MAX)

>>>>

>>>> the argument will get sliced (and trigger a -Woverflow).  One will

>>>> need to go back to the more verbose way of calling it.

>>>

>>> The int version is not really meant to pass anything but simple

>>> constants.  For anything fancy, one should really be using the tree

>>> version.  But I can certainly change the argument to HOST_WIDE_INT if

>>> preferred.

>>>

>>>>

>>>> Changing the argument type to HOST_WIDE_INT would avoid the slicing

>>>> and warning but then make contains_p (0) ambiguous because 0 isn't

>>>> a perfect match for either void* or long (so it requires a conversion).

>>>

>>> Just a plain 0 will match the int version, instead of the tree version,

>>> right?  Nobody should be passing NULL to the tree version, so that seems

>>> like a non-issue.

>>

>> Right, NULL isn't a problem, but I would expect any integer to work

>> (I thought that's what Richard was asking for)  So my suggestion was

>> to have contains_p() a poly_int64 and provide just as robust an API

>> as build_int_cst.  The argument ends up converted to the poly_int64

>> anyway when it's passed to the latter.  I.e., why not define

>> contains_p simply like this:

>>

>>     bool

>>     value_range_base::contains_p (poly_int64 val) const

>>     {

>>       if (varying_p ())

>>         return true;

>>       if (undefined_p ())

>>         return false;

>>

>>       return contains_p (build_int_cst (type (), val));

>>     }

> 

> I agree that plain 'int' is bad.  Given we currently store

> INTEGER_CSTs only (and not POLY_INT_CSTs) a

> widest_int argument should be fine.  Note widest

> because when interpreted signed all unsigned values

> fit.

> 

> The existing contains_p check is also a bit fishy

> for the cases TREE_TYPE of the value has a

> value-range not covered in the value-ranges type...

> I guess it could do

> 

>     if (!int_fits_type_p (val, this->type ())

>       return false;

> 

> but that changes existing behavior which happily

> throws different sign constants into tree_int_cst_lt

> for example...  Do we want to allow non-INTEGER_CST

> val arguments at all?  That is, you make contains_p

> return a bool and say "yes" when we couldn't really

> decide.  This is why previously it was named

> may_contain_p (and we didn't have must_contain_p).

> I think making the result explicitely clear is a must

> since contains_p reads like !contains_p means

> it does _not_ contain.  So, either change back the

> name (and provide the must variant)

> or make it return the tri-state from value_inside_range.


The only reason I added the int overload was because you didn't like the 
build_one_cst call.  However, if we make it HOST_WIDE_INT or even 
widest_int, it will cause a conflict resolution with the tree version. 
I think it's best we only provide a tree version, and make due with the 
one call to build_one_cst.  There is still the range_includes_zero_p() 
convenience function that takes care of the common case.

I see what you mean with the may/must problem.  We don't have that 
problem in the ranger because we only deal with constants, and there is 
never ambiguity.  Since we don't have any consumers of must_contain_p 
yet, let's leave this bit for later discussion when it's actually needed.

What I propose then is a general clean up of may_contain_p, making 
value_inside_range a member function that deals with the range in THIS. 
We can remove value_inside_range, as it's only used from the one 
build_on-cst place, and range_includes_zero_p (which I've adjusted 
accordingly).

Note.  I've changed existing functionality to so that may_contains_p 
(and range_includes_zero_p accordingly) returns FALSE for VR_UNDEFINED. 
AFAIU, VR_UNDEFINED is the empty set, so we absolutely know it's not 
inside the range.  I've tested my patch both with and without this 
VR_UNDEFINED changelet, and there are no regressions.

With this cleanup, particularly with rewriting may_contain_p in terms of 
the now value_inside_range method, we can later provide must_contain_p 
when needed with just:

bool
value_range_base::must_contain_p (tree val) const
{
   return value_inside_range (val) == 1;
}

OK?
gcc/

	* gimple-loop-versioning.cc (prune_loop_conditions): Use
	may_contain_p.
	* tree-vrp (value_range_base::may_contain_p): Call into
	value_inside_range.
	(value_inside_range): Make private inside value_range_base class.
	Take min/max from *this.
	(range_includes_p): Remove.
	* tree-vrp.h (value_range_base): Add value_inside_range.
	(range_includes_p): Remove.
	(range_includes_zero_p): Call may_contain_p.
	* vr-values.c (compare_range_with_value): Same.

diff --git a/gcc/gimple-loop-versioning.cc b/gcc/gimple-loop-versioning.cc
index 2f7cda9c8cb..fe273001423 100644
--- a/gcc/gimple-loop-versioning.cc
+++ b/gcc/gimple-loop-versioning.cc
@@ -1488,7 +1488,7 @@ loop_versioning::prune_loop_conditions (struct loop *loop, vr_values *vrs)
     {
       tree name = ssa_name (i);
       value_range *vr = vrs->get_value_range (name);
-      if (vr && !range_includes_p (vr, 1))
+      if (vr && !vr->may_contain_p (build_one_cst (TREE_TYPE (name))))
 	{
 	  if (dump_enabled_p ())
 	    dump_printf_loc (MSG_NOTE, find_loop_location (loop),
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 065152aca3b..dc7f825efc8 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -287,18 +287,7 @@ value_range::set_varying ()
 bool
 value_range_base::may_contain_p (tree val) const
 {
-  if (varying_p ())
-    return true;
-
-  if (undefined_p ())
-    return true;
-
-  if (m_kind == VR_ANTI_RANGE)
-    {
-      int res = value_inside_range (val, min (), max ());
-      return res == 0 || res == -2;
-    }
-  return value_inside_range (val, min (), max ()) != 0;
+  return value_inside_range (val) != 0;
 }
 
 void
@@ -1118,40 +1107,38 @@ compare_values (tree val1, tree val2)
 }
 
 
-/* Return 1 if VAL is inside value range MIN <= VAL <= MAX,
-          0 if VAL is not inside [MIN, MAX],
+/* Return 1 if VAL is inside value range.
+          0 if VAL is not inside value range.
 	 -2 if we cannot tell either way.
 
    Benchmark compile/20001226-1.c compilation time after changing this
    function.  */
 
 int
-value_inside_range (tree val, tree min, tree max)
+value_range_base::value_inside_range (tree val) const
 {
   int cmp1, cmp2;
 
-  cmp1 = operand_less_p (val, min);
+  if (varying_p ())
+    return 1;
+
+  if (undefined_p ())
+    return 0;
+
+  cmp1 = operand_less_p (val, m_min);
   if (cmp1 == -2)
     return -2;
   if (cmp1 == 1)
-    return 0;
+    return m_kind != VR_RANGE;
 
-  cmp2 = operand_less_p (max, val);
+  cmp2 = operand_less_p (m_max, val);
   if (cmp2 == -2)
     return -2;
 
-  return !cmp2;
-}
-
-
-/* Return TRUE if *VR includes the value X.  */
-
-bool
-range_includes_p (const value_range_base *vr, HOST_WIDE_INT x)
-{
-  if (vr->varying_p () || vr->undefined_p ())
-    return true;
-  return vr->may_contain_p (build_int_cst (vr->type (), x));
+  if (m_kind == VR_RANGE)
+    return !cmp2;
+  else
+    return !!cmp2;
 }
 
 /* Value range wrapper for wide_int_range_set_zero_nonzero_bits.
diff --git a/gcc/tree-vrp.h b/gcc/tree-vrp.h
index 62d72868708..4ec974f5fdb 100644
--- a/gcc/tree-vrp.h
+++ b/gcc/tree-vrp.h
@@ -97,6 +97,9 @@ protected:
   friend void gt_ggc_mx (value_range_base *&);
   friend void gt_pch_nx (value_range_base &);
   friend void gt_pch_nx (value_range_base *, gt_pointer_operator, void *);
+
+private:
+  int value_inside_range (tree) const;
 };
 
 /* Note value_range cannot currently be used with GC memory, only
@@ -254,7 +257,6 @@ struct assert_info
 extern void register_edge_assert_for (tree, edge, enum tree_code,
 				      tree, tree, vec<assert_info> &);
 extern bool stmt_interesting_for_vrp (gimple *);
-extern bool range_includes_p (const value_range_base *, HOST_WIDE_INT);
 extern bool infer_value_range (gimple *, tree, tree_code *, tree *);
 
 extern bool vrp_bitmap_equal_p (const_bitmap, const_bitmap);
@@ -267,7 +269,6 @@ extern int compare_values_warnv (tree, tree, bool *);
 extern int operand_less_p (tree, tree);
 extern bool vrp_val_is_min (const_tree);
 extern bool vrp_val_is_max (const_tree);
-extern int value_inside_range (tree, tree, tree);
 
 extern tree vrp_val_min (const_tree);
 extern tree vrp_val_max (const_tree);
@@ -300,7 +301,13 @@ extern value_range_kind determine_value_range (tree, wide_int *, wide_int *);
 inline bool
 range_includes_zero_p (const value_range_base *vr)
 {
-  return range_includes_p (vr, 0);
+  if (vr->undefined_p ())
+    return false;
+
+  if (vr->varying_p ())
+    return true;
+
+  return vr->may_contain_p (build_zero_cst (vr->type ()));
 }
 
 #endif /* GCC_TREE_VRP_H */
diff --git a/gcc/vr-values.c b/gcc/vr-values.c
index e151550d963..3f20c1a6fe8 100644
--- a/gcc/vr-values.c
+++ b/gcc/vr-values.c
@@ -1625,7 +1625,7 @@ compare_range_with_value (enum tree_code comp, value_range *vr, tree val,
 	return NULL_TREE;
 
       /* ~[VAL_1, VAL_2] OP VAL is known if VAL_1 <= VAL <= VAL_2.  */
-      if (value_inside_range (val, vr->min (), vr->max ()) == 1)
+      if (!vr->may_contain_p (val))
 	return (comp == NE_EXPR) ? boolean_true_node : boolean_false_node;
 
       return NULL_TREE;
Richard Biener June 13, 2019, 8:27 a.m. | #10
On Wed, Jun 12, 2019 at 11:33 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>

> On 6/12/19 5:33 AM, Richard Biener wrote:

> > On Wed, Jun 12, 2019 at 1:57 AM Martin Sebor <msebor@gmail.com> wrote:

> >>

> >> On 6/11/19 3:02 PM, Aldy Hernandez wrote:

> >>>

> >>>

> >>> On 6/11/19 12:52 PM, Martin Sebor wrote:

> >>>> On 6/11/19 10:26 AM, Aldy Hernandez wrote:

> >>>>>

> >>>>>

> >>>>> On 6/11/19 12:17 PM, Martin Sebor wrote:

> >>>>>> On 6/11/19 9:05 AM, Aldy Hernandez wrote:

> >>>>>>>

> >>>>>>>

> >>>>>>> On 6/11/19 9:45 AM, Richard Biener wrote:

> >>>>>>>> On Tue, Jun 11, 2019 at 12:40 PM Aldy Hernandez <aldyh@redhat.com>

> >>>>>>>> wrote:

> >>>>>>>>>

> >>>>>>>>> This patch cleans up the various contains, may_contain, and

> >>>>>>>>> value_inside_range variants we have throughout, in favor of one--

> >>>>>>>>> contains_p.  There should be no changes in functionality.

> >>>>>>>>>

> >>>>>>>>> I have added a note to range_includes_zero_p, perhaps as a personal

> >>>>>>>>> question than anything else.  This function was/is returning true

> >>>>>>>>> for

> >>>>>>>>> UNDEFINED.  From a semantic sense, that doesn't make sense.

> >>>>>>>>> UNDEFINED

> >>>>>>>>> is really the empty set.  Is the functionality wrong, or should

> >>>>>>>>> we call

> >>>>>>>>> this function something else?  Either way, I'm fine removing the

> >>>>>>>>> comment

> >>>>>>>>> but I'm genuinely curious.

> >>>>>>>>

> >>>>>>>> So this affects for example this hunk:

> >>>>>>>>

> >>>>>>>> -      if (vr && !range_includes_p (vr, 1))

> >>>>>>>> +      if (vr && (!vr->contains_p (build_one_cst (TREE_TYPE (name)))

> >>>>>>>> +                && !vr->undefined_p ()))

> >>>>>>>>           {

> >>>>>>>>

> >>>>>>>> I think it's arbitrary how we compute X in UNDEFINED and I'm fine

> >>>>>>>> with changing the affected predicates to return false.  This means

> >>>>>>>> not testing for !vr->undefined_p here.

> >>>>>>>

> >>>>>>> Excellent.

> >>>>>>>

> >>>>>>>>

> >>>>>>>> Note I very much dislike the build_one_cst () call here so please

> >>>>>>>> provide an overload hiding this.

> >>>>>>>

> >>>>>>> Good idea.  I love it.

> >>>>>>>

> >>>>>>>>

> >>>>>>>> Not sure why you keep range_includes_zero_p.

> >>>>>>>

> >>>>>>> I wasn't sure if there was some subtle reason why we were including

> >>>>>>> undefined.

> >>>>>>>

> >>>>>>> OK pending tests?

> >>>>>>

> >>>>>> Should the second overload:

> >>>>>>

> >>>>>> +  bool contains_p (tree) const;

> >>>>>> +  bool contains_p (int) const;

> >>>>>>

> >>>>>> take something like HOST_WIDE_INT or even one of those poly_ints

> >>>>>> like build_int_cst does?  (With the former, contains_p (0) will

> >>>>>> likely be ambiguous since 0 is int and HOST_WIDE_INT is long).

> >>>>>

> >>>>> We have a type, so there should be no confusion:

> >>>>>

> >>>>> +  return contains_p (build_int_cst (type (), val));

> >>>>>

> >>>>> (UNDEFINED and VARYING don't have a type, so they are special cased

> >>>>> prior).

> >>>>

> >>>> I didn't mean the overloads are confusing, just that there the one

> >>>> that takes an int doesn't make it possible to test whether a value

> >>>> outside the range of an int is in the range.  For example, in

> >>>> the call

> >>>>

> >>>>     contains_p (SIZE_MAX)

> >>>>

> >>>> the argument will get sliced (and trigger a -Woverflow).  One will

> >>>> need to go back to the more verbose way of calling it.

> >>>

> >>> The int version is not really meant to pass anything but simple

> >>> constants.  For anything fancy, one should really be using the tree

> >>> version.  But I can certainly change the argument to HOST_WIDE_INT if

> >>> preferred.

> >>>

> >>>>

> >>>> Changing the argument type to HOST_WIDE_INT would avoid the slicing

> >>>> and warning but then make contains_p (0) ambiguous because 0 isn't

> >>>> a perfect match for either void* or long (so it requires a conversion).

> >>>

> >>> Just a plain 0 will match the int version, instead of the tree version,

> >>> right?  Nobody should be passing NULL to the tree version, so that seems

> >>> like a non-issue.

> >>

> >> Right, NULL isn't a problem, but I would expect any integer to work

> >> (I thought that's what Richard was asking for)  So my suggestion was

> >> to have contains_p() a poly_int64 and provide just as robust an API

> >> as build_int_cst.  The argument ends up converted to the poly_int64

> >> anyway when it's passed to the latter.  I.e., why not define

> >> contains_p simply like this:

> >>

> >>     bool

> >>     value_range_base::contains_p (poly_int64 val) const

> >>     {

> >>       if (varying_p ())

> >>         return true;

> >>       if (undefined_p ())

> >>         return false;

> >>

> >>       return contains_p (build_int_cst (type (), val));

> >>     }

> >

> > I agree that plain 'int' is bad.  Given we currently store

> > INTEGER_CSTs only (and not POLY_INT_CSTs) a

> > widest_int argument should be fine.  Note widest

> > because when interpreted signed all unsigned values

> > fit.

> >

> > The existing contains_p check is also a bit fishy

> > for the cases TREE_TYPE of the value has a

> > value-range not covered in the value-ranges type...

> > I guess it could do

> >

> >     if (!int_fits_type_p (val, this->type ())

> >       return false;

> >

> > but that changes existing behavior which happily

> > throws different sign constants into tree_int_cst_lt

> > for example...  Do we want to allow non-INTEGER_CST

> > val arguments at all?  That is, you make contains_p

> > return a bool and say "yes" when we couldn't really

> > decide.  This is why previously it was named

> > may_contain_p (and we didn't have must_contain_p).

> > I think making the result explicitely clear is a must

> > since contains_p reads like !contains_p means

> > it does _not_ contain.  So, either change back the

> > name (and provide the must variant)

> > or make it return the tri-state from value_inside_range.

>

> The only reason I added the int overload was because you didn't like the

> build_one_cst call.  However, if we make it HOST_WIDE_INT or even

> widest_int, it will cause a conflict resolution with the tree version.

> I think it's best we only provide a tree version, and make due with the

> one call to build_one_cst.  There is still the range_includes_zero_p()

> convenience function that takes care of the common case.

>

> I see what you mean with the may/must problem.  We don't have that

> problem in the ranger because we only deal with constants, and there is

> never ambiguity.  Since we don't have any consumers of must_contain_p

> yet, let's leave this bit for later discussion when it's actually needed.

>

> What I propose then is a general clean up of may_contain_p, making

> value_inside_range a member function that deals with the range in THIS.

> We can remove value_inside_range, as it's only used from the one

> build_on-cst place, and range_includes_zero_p (which I've adjusted

> accordingly).

>

> Note.  I've changed existing functionality to so that may_contains_p

> (and range_includes_zero_p accordingly) returns FALSE for VR_UNDEFINED.

> AFAIU, VR_UNDEFINED is the empty set, so we absolutely know it's not

> inside the range.  I've tested my patch both with and without this

> VR_UNDEFINED changelet, and there are no regressions.

>

> With this cleanup, particularly with rewriting may_contain_p in terms of

> the now value_inside_range method, we can later provide must_contain_p

> when needed with just:

>

> bool

> value_range_base::must_contain_p (tree val) const

> {

>    return value_inside_range (val) == 1;

> }

>

> OK?


OK.  This is definitely an obvious improvement.  As you say we can
do more adjustments later.

Richard.

Patch

commit eac5cbfa507146166498e395d9347efdf46d3ef4
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Tue Jun 4 09:11:22 2019 +0200

    value_range_base::contains_p and may_contains_p.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index bee0a75a000..4f5c1798751 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,17 @@ 
+2019-06-11  Aldy Hernandez  <aldyh@redhat.com>
+
+	* gimple-loop-versioning.cc (prune_loop_conditions): Use
+	value_range_base::contains_p.
+	* tree-vrp.c (value_range_base::may_contain_p): Remove.
+	(value_inside_range): Make private to value_range_base class.
+	(value_range_base::contains_p): New.
+	* tree-vrp.h (value_range_base): Remove may_contain_p.  Add
+	contains_p.  Add value_inside_range.
+	(range_includes_p): Remove.
+	(value_inside_range): Remove.
+	(range_includes_p): Use value_range_base::contains_p.
+	* vr-values.c (compare_range_with_value): Same.
+
 2019-06-11  Aldy Hernandez  <aldyh@redhat.com>
 
 	* gimple-ssa-evrp.c (evrp_dom_walker::before_dom_children): Use
diff --git a/gcc/gimple-loop-versioning.cc b/gcc/gimple-loop-versioning.cc
index a2f68bf08db..85468fa06c4 100644
--- a/gcc/gimple-loop-versioning.cc
+++ b/gcc/gimple-loop-versioning.cc
@@ -1489,7 +1489,8 @@  loop_versioning::prune_loop_conditions (struct loop *loop, vr_values *vrs)
     {
       tree name = ssa_name (i);
       value_range *vr = vrs->get_value_range (name);
-      if (vr && !range_includes_p (vr, 1))
+      if (vr && (!vr->contains_p (build_one_cst (TREE_TYPE (name)))
+		 && !vr->undefined_p ()))
 	{
 	  if (dump_enabled_p ())
 	    dump_printf_loc (MSG_NOTE, find_loop_location (loop),
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 08e6c6b6111..58ca9fa0d34 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -292,25 +292,6 @@  value_range::set_varying (tree type)
   equiv_clear ();
 }
 
-/* Return TRUE if it is possible that range contains VAL.  */
-
-bool
-value_range_base::may_contain_p (tree val) const
-{
-  if (varying_p ())
-    return true;
-
-  if (undefined_p ())
-    return true;
-
-  if (m_kind == VR_ANTI_RANGE)
-    {
-      int res = value_inside_range (val, min (), max ());
-      return res == 0 || res == -2;
-    }
-  return value_inside_range (val, min (), max ()) != 0;
-}
-
 void
 value_range::equiv_clear ()
 {
@@ -1160,7 +1141,7 @@  compare_values (tree val1, tree val2)
    function.  */
 
 int
-value_inside_range (tree val, tree min, tree max)
+value_range_base::value_inside_range (tree val, tree min, tree max) const
 {
   int cmp1, cmp2;
 
@@ -1177,15 +1158,23 @@  value_inside_range (tree val, tree min, tree max)
   return !cmp2;
 }
 
-
-/* Return TRUE if *VR includes the value X.  */
+/* Return TRUE if range contains VAL.  */
 
 bool
-range_includes_p (const value_range_base *vr, HOST_WIDE_INT x)
+value_range_base::contains_p (tree val) const
 {
-  if (vr->varying_p () || vr->undefined_p ())
+  if (varying_p ())
     return true;
-  return vr->may_contain_p (build_int_cst (vr->type (), x));
+
+  if (undefined_p ())
+    return false;
+
+  if (m_kind == VR_ANTI_RANGE)
+    {
+      int res = value_inside_range (val, min (), max ());
+      return res == 0 || res == -2;
+    }
+  return value_inside_range (val, min (), max ()) != 0;
 }
 
 /* Value range wrapper for wide_int_range_set_zero_nonzero_bits.
diff --git a/gcc/tree-vrp.h b/gcc/tree-vrp.h
index e48cc37bafc..3abe0f7fe98 100644
--- a/gcc/tree-vrp.h
+++ b/gcc/tree-vrp.h
@@ -70,7 +70,7 @@  public:
 
   /* Misc methods.  */
   tree type () const;
-  bool may_contain_p (tree) const;
+  bool contains_p (tree) const;
   void set_and_canonicalize (enum value_range_kind, tree, tree);
   bool zero_p () const;
   bool nonzero_p () const;
@@ -97,6 +97,9 @@  protected:
   friend void gt_ggc_mx (value_range_base *&);
   friend void gt_pch_nx (value_range_base &);
   friend void gt_pch_nx (value_range_base *, gt_pointer_operator, void *);
+
+private:
+  int value_inside_range (tree, tree, tree) const;
 };
 
 /* Note value_range cannot currently be used with GC memory, only
@@ -254,7 +257,6 @@  struct assert_info
 extern void register_edge_assert_for (tree, edge, enum tree_code,
 				      tree, tree, vec<assert_info> &);
 extern bool stmt_interesting_for_vrp (gimple *);
-extern bool range_includes_p (const value_range_base *, HOST_WIDE_INT);
 extern bool infer_value_range (gimple *, tree, tree_code *, tree *);
 
 extern bool vrp_bitmap_equal_p (const_bitmap, const_bitmap);
@@ -267,7 +269,6 @@  extern int compare_values_warnv (tree, tree, bool *);
 extern int operand_less_p (tree, tree);
 extern bool vrp_val_is_min (const_tree);
 extern bool vrp_val_is_max (const_tree);
-extern int value_inside_range (tree, tree, tree);
 
 extern tree vrp_val_min (const_tree);
 extern tree vrp_val_max (const_tree);
@@ -300,7 +301,11 @@  extern value_range_kind determine_value_range (tree, wide_int *, wide_int *);
 inline bool
 range_includes_zero_p (const value_range_base *vr)
 {
-  return range_includes_p (vr, 0);
+  /* ?? I'm not convinced we should return TRUE for undefined.  */
+  /* Short-circuit these because they don't have a type.  */
+  if (vr->undefined_p () || vr->varying_p ())
+    return true;
+  return vr->contains_p (build_zero_cst (vr->type ()));
 }
 
 #endif /* GCC_TREE_VRP_H */
diff --git a/gcc/vr-values.c b/gcc/vr-values.c
index 1c05ffb7f17..77b036a8ae6 100644
--- a/gcc/vr-values.c
+++ b/gcc/vr-values.c
@@ -1626,8 +1626,7 @@  compare_range_with_value (enum tree_code comp, value_range *vr, tree val,
 	  || comp == LE_EXPR)
 	return NULL_TREE;
 
-      /* ~[VAL_1, VAL_2] OP VAL is known if VAL_1 <= VAL <= VAL_2.  */
-      if (value_inside_range (val, vr->min (), vr->max ()) == 1)
+      if (!vr->contains_p (val))
 	return (comp == NE_EXPR) ? boolean_true_node : boolean_false_node;
 
       return NULL_TREE;