diff mbox series

value_range_base::{non_zero_p, set_zero, set_non_zero}

Message ID af11453b-e355-2896-2cff-8164c44630d1@redhat.com
State New
Headers show
Series value_range_base::{non_zero_p, set_zero, set_non_zero} | expand

Commit Message

Aldy Hernandez May 30, 2019, 6:58 a.m. UTC 
Hi.

We have zero_p in the API, but we don't have non_zero_p.  Instead we use 
a non-API function range_is_nonnull.  I've fixed this.

I have also gotten rid of the duplicity of using the non-API 
range_is_null in favor of value_range_base::zero_p().

Furthermore, there's value_range*::set_null and 
value_range*::set_nonnull().  It's inconsistent to use null/nonnull as 
well as zero/non_zero throughout.  I've moved everything to *zero.

Finally, it seems to me that the derived value_range versions of 
set_*zero/null are a bit confusing in that they clear equivalences 
behind the scenes.  There's no intuitive reason why setting a range of 
[0,0] versus [5,10] should clear equivalences.  I've made the 
equivalence nuking explicit in the handful of places where we do this, 
and thus reduced the need for separate value_range versions.

I believe with these changes, as well as the pending intersect patch, 
we've cleaned up the remaining value_range uses where we actually wanted 
to use value_range_base.  Or at least the remaining "value_range tem" 
business.

OK?

Aldy

Comments

Jeff Law May 30, 2019, 11:54 p.m. UTC | #1 
On 5/30/19 12:58 AM, Aldy Hernandez wrote:
> Hi.
> 
> We have zero_p in the API, but we don't have non_zero_p.  Instead we use
> a non-API function range_is_nonnull.  I've fixed this.
> 
> I have also gotten rid of the duplicity of using the non-API
> range_is_null in favor of value_range_base::zero_p().
> 
> Furthermore, there's value_range*::set_null and
> value_range*::set_nonnull().  It's inconsistent to use null/nonnull as
> well as zero/non_zero throughout.  I've moved everything to *zero.
> 
> Finally, it seems to me that the derived value_range versions of
> set_*zero/null are a bit confusing in that they clear equivalences
> behind the scenes.  There's no intuitive reason why setting a range of
> [0,0] versus [5,10] should clear equivalences.  I've made the
> equivalence nuking explicit in the handful of places where we do this,
> and thus reduced the need for separate value_range versions.
> 
> I believe with these changes, as well as the pending intersect patch,
> we've cleaned up the remaining value_range uses where we actually wanted
> to use value_range_base.  Or at least the remaining "value_range tem"
> business.
> 
> OK?
> 
> Aldy
> 
> curr.patch
> 
> commit 55294d340a0727dbe985ee4bf3c1969a19bcbe6d
> Author: Aldy Hernandez <aldyh@redhat.com>
> Date:   Tue May 28 19:30:31 2019 +0200
> 
>             * tree-vrp.h (value_range_base::non_zero_p): New.
>             (value_range_base::set_nonnull): Rename to...
>             (value_range_base::set_non_zero): ...this.
>             (value_range_base::set_null): Rename to...
>             (value_range_base::set_zero): ...this.
>             (value_range::set_nonnull): Remove.
>             (value_range::set_null): Remove.
>             * tree-vrp.c (range_is_null): Remove.
>             (range_is_nonnull): Remove.
>             (extract_range_from_binary_expr): Use value_range_base::*zero_p
>             instead of range_is_*null.
>             (extract_range_from_unary_expr): Same.
>             (value_range_base::set_nonnull): Rename to...
>             (value_range_base::set_non_zero): ...this.
>             (value_range::set_nonnull): Remove.
>             (value_range_base::set_null): Rename to...
>             (value_range_base::set_zero): ...this.
>             (value_range::set_null): Remove.
>             (extract_range_from_binary_expr): Rename set_*null uses to
>             set_*zero.
>             (extract_range_from_unary_expr): Same.
>             (union_helper): Same.
>             * vr-values.c (get_value_range): Use set_*zero instead of
>             set_*null.
>             (vr_values::extract_range_from_binary_expr): Same.
>             (vr_values::extract_range_basic): Same.
> 
OK
jeff
Martin Sebor May 31, 2019, 12:25 a.m. UTC | #2 
On 5/30/19 12:58 AM, Aldy Hernandez wrote:
> Hi.
> 
> We have zero_p in the API, but we don't have non_zero_p.  Instead we use 
> a non-API function range_is_nonnull.  I've fixed this.
> 
> I have also gotten rid of the duplicity of using the non-API 
> range_is_null in favor of value_range_base::zero_p().
> 
> Furthermore, there's value_range*::set_null and 
> value_range*::set_nonnull().  It's inconsistent to use null/nonnull as 
> well as zero/non_zero throughout.  I've moved everything to *zero.

With the -Wformat-diag cleanup still fresh in my memory, I can't
help but point out that the GCC spelling convention calls for
"nonzero" vs "non-zero" or "non zero".

Naming the function set_nonzero() would be in line with both
the convention and established practice (over 2000 instances)
and set_non_zero would not be (only 22 instances of non_zero
in GCC sources).

This, of course, is in contrast to things like bit-field and
built-in where the convention calls for the hyphen but where
in code we seem to prefer "bitfield" nonetheless ;-) (Names
like get_bit_field_ref_def and bit_field_size being
the exceptions).

Martin

> 
> Finally, it seems to me that the derived value_range versions of 
> set_*zero/null are a bit confusing in that they clear equivalences 
> behind the scenes.  There's no intuitive reason why setting a range of 
> [0,0] versus [5,10] should clear equivalences.  I've made the 
> equivalence nuking explicit in the handful of places where we do this, 
> and thus reduced the need for separate value_range versions.
> 
> I believe with these changes, as well as the pending intersect patch, 
> we've cleaned up the remaining value_range uses where we actually wanted 
> to use value_range_base.  Or at least the remaining "value_range tem" 
> business.
> 
> OK?
> 
> Aldy
Joseph Myers May 31, 2019, 1:09 a.m. UTC | #3 
On Thu, 30 May 2019, Martin Sebor wrote:

> This, of course, is in contrast to things like bit-field and
> built-in where the convention calls for the hyphen but where

For both bit-field and nonzero what we do in documentation is consistent 
with the C standard, even if code is less consistent.  (grep on the C 
standard sources shows 148 lines matching for nonzero and only 2 for 
non-zero.)
Aldy Hernandez May 31, 2019, 9:02 a.m. UTC | #4 
Thanks. I will adjust accordingly.

On Fri, May 31, 2019, 02:26 Martin Sebor <msebor@gmail.com> wrote:

> On 5/30/19 12:58 AM, Aldy Hernandez wrote:
> > Hi.
> >
> > We have zero_p in the API, but we don't have non_zero_p.  Instead we use
> > a non-API function range_is_nonnull.  I've fixed this.
> >
> > I have also gotten rid of the duplicity of using the non-API
> > range_is_null in favor of value_range_base::zero_p().
> >
> > Furthermore, there's value_range*::set_null and
> > value_range*::set_nonnull().  It's inconsistent to use null/nonnull as
> > well as zero/non_zero throughout.  I've moved everything to *zero.
>
> With the -Wformat-diag cleanup still fresh in my memory, I can't
> help but point out that the GCC spelling convention calls for
> "nonzero" vs "non-zero" or "non zero".
>
> Naming the function set_nonzero() would be in line with both
> the convention and established practice (over 2000 instances)
> and set_non_zero would not be (only 22 instances of non_zero
> in GCC sources).
>
> This, of course, is in contrast to things like bit-field and
> built-in where the convention calls for the hyphen but where
> in code we seem to prefer "bitfield" nonetheless ;-) (Names
> like get_bit_field_ref_def and bit_field_size being
> the exceptions).
>
> Martin
>
> >
> > Finally, it seems to me that the derived value_range versions of
> > set_*zero/null are a bit confusing in that they clear equivalences
> > behind the scenes.  There's no intuitive reason why setting a range of
> > [0,0] versus [5,10] should clear equivalences.  I've made the
> > equivalence nuking explicit in the handful of places where we do this,
> > and thus reduced the need for separate value_range versions.
> >
> > I believe with these changes, as well as the pending intersect patch,
> > we've cleaned up the remaining value_range uses where we actually wanted
> > to use value_range_base.  Or at least the remaining "value_range tem"
> > business.
> >
> > OK?
> >
> > Aldy
>
>
diff mbox series

Patch

commit 55294d340a0727dbe985ee4bf3c1969a19bcbe6d
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Tue May 28 19:30:31 2019 +0200

            * tree-vrp.h (value_range_base::non_zero_p): New.
            (value_range_base::set_nonnull): Rename to...
            (value_range_base::set_non_zero): ...this.
            (value_range_base::set_null): Rename to...
            (value_range_base::set_zero): ...this.
            (value_range::set_nonnull): Remove.
            (value_range::set_null): Remove.
            * tree-vrp.c (range_is_null): Remove.
            (range_is_nonnull): Remove.
            (extract_range_from_binary_expr): Use value_range_base::*zero_p
            instead of range_is_*null.
            (extract_range_from_unary_expr): Same.
            (value_range_base::set_nonnull): Rename to...
            (value_range_base::set_non_zero): ...this.
            (value_range::set_nonnull): Remove.
            (value_range_base::set_null): Rename to...
            (value_range_base::set_zero): ...this.
            (value_range::set_null): Remove.
            (extract_range_from_binary_expr): Rename set_*null uses to
            set_*zero.
            (extract_range_from_unary_expr): Same.
            (union_helper): Same.
            * vr-values.c (get_value_range): Use set_*zero instead of
            set_*null.
            (vr_values::extract_range_from_binary_expr): Same.
            (vr_values::extract_range_basic): Same.

diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 0a172719e5d..ef0ed97748b 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -776,32 +776,19 @@  value_range::set (tree val)
   set (VR_RANGE, val, val, NULL);
 }
 
-/* Set value range VR to a non-NULL range of type TYPE.  */
+/* Set value range VR to a non-zero range of type TYPE.  */
 
 void
-value_range_base::set_nonnull (tree type)
+value_range_base::set_non_zero (tree type)
 {
   tree zero = build_int_cst (type, 0);
   set (VR_ANTI_RANGE, zero, zero);
 }
 
-void
-value_range::set_nonnull (tree type)
-{
-  tree zero = build_int_cst (type, 0);
-  set (VR_ANTI_RANGE, zero, zero, NULL);
-}
-
-/* Set value range VR to a NULL range of type TYPE.  */
-
-void
-value_range_base::set_null (tree type)
-{
-  set (build_int_cst (type, 0));
-}
+/* Set value range VR to a ZERO range of type TYPE.  */
 
 void
-value_range::set_null (tree type)
+value_range_base::set_zero (tree type)
 {
   set (build_int_cst (type, 0));
 }
@@ -830,22 +817,6 @@  vrp_bitmap_equal_p (const_bitmap b1, const_bitmap b2)
 	      && bitmap_equal_p (b1, b2)));
 }
 
-/* Return true if VR is [0, 0].  */
-
-static inline bool
-range_is_null (const value_range_base *vr)
-{
-  return vr->zero_p ();
-}
-
-static inline bool
-range_is_nonnull (const value_range_base *vr)
-{
-  return (vr->kind () == VR_ANTI_RANGE
-	  && vr->min () == vr->max ()
-	  && integer_zerop (vr->min ()));
-}
-
 /* Return true if max and min of VR are INTEGER_CST.  It's not necessary
    a singleton.  */
 
@@ -1583,9 +1554,9 @@  extract_range_from_binary_expr (value_range_base *vr,
      code is EXACT_DIV_EXPR.  We could mask out bits in the resulting
      range, but then we also need to hack up vrp_union.  It's just
      easier to special case when vr0 is ~[0,0] for EXACT_DIV_EXPR.  */
-  if (code == EXACT_DIV_EXPR && range_is_nonnull (&vr0))
+  if (code == EXACT_DIV_EXPR && vr0.non_zero_p ())
     {
-      vr->set_nonnull (expr_type);
+      vr->set_non_zero (expr_type);
       return;
     }
 
@@ -1663,9 +1634,9 @@  extract_range_from_binary_expr (value_range_base *vr,
 	     If both are null, then the result is null. Otherwise they
 	     are varying.  */
 	  if (!range_includes_zero_p (&vr0) && !range_includes_zero_p (&vr1))
-	    vr->set_nonnull (expr_type);
-	  else if (range_is_null (&vr0) && range_is_null (&vr1))
-	    vr->set_null (expr_type);
+	    vr->set_non_zero (expr_type);
+	  else if (vr0.zero_p () && vr1.zero_p ())
+	    vr->set_zero (expr_type);
 	  else
 	    vr->set_varying ();
 	}
@@ -1692,9 +1663,9 @@  extract_range_from_binary_expr (value_range_base *vr,
 	      && (flag_delete_null_pointer_checks
 		  || (range_int_cst_p (&vr1)
 		      && !tree_int_cst_sign_bit (vr1.max ()))))
-	    vr->set_nonnull (expr_type);
-	  else if (range_is_null (&vr0) && range_is_null (&vr1))
-	    vr->set_null (expr_type);
+	    vr->set_non_zero (expr_type);
+	  else if (vr0.zero_p () && vr1.zero_p ())
+	    vr->set_zero (expr_type);
 	  else
 	    vr->set_varying ();
 	}
@@ -1703,9 +1674,9 @@  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))
-	    vr->set_nonnull (expr_type);
-	  else if (range_is_null (&vr0) || range_is_null (&vr1))
-	    vr->set_null (expr_type);
+	    vr->set_non_zero (expr_type);
+	  else if (vr0.zero_p () || vr1.zero_p ())
+	    vr->set_zero (expr_type);
 	  else
 	    vr->set_varying ();
 	}
@@ -1898,7 +1869,7 @@  extract_range_from_binary_expr (value_range_base *vr,
       bool extra_range_p;
 
       /* Special case explicit division by zero as undefined.  */
-      if (range_is_null (&vr1))
+      if (vr1.zero_p ())
 	{
 	  vr->set_undefined ();
 	  return;
@@ -1937,7 +1908,7 @@  extract_range_from_binary_expr (value_range_base *vr,
     }
   else if (code == TRUNC_MOD_EXPR)
     {
-      if (range_is_null (&vr1))
+      if (vr1.zero_p ())
 	{
 	  vr->set_undefined ();
 	  return;
@@ -2141,9 +2112,9 @@  extract_range_from_unary_expr (value_range_base *vr,
       if (POINTER_TYPE_P (type) || POINTER_TYPE_P (op0_type))
 	{
 	  if (!range_includes_zero_p (&vr0))
-	    vr->set_nonnull (type);
-	  else if (range_is_null (&vr0))
-	    vr->set_null (type);
+	    vr->set_non_zero (type);
+	  else if (vr0.zero_p ())
+	    vr->set_zero (type);
 	  else
 	    vr->set_varying ();
 	  return;
@@ -6152,7 +6123,7 @@  value_range_base::union_helper (const value_range_base *vr0,
 		vr1->kind (), vr1->min (), vr1->max ());
 
   /* Work on a temporary so we can still use vr0 when union returns varying.  */
-  value_range tem;
+  value_range_base tem;
   tem.set_and_canonicalize (vr0type, vr0min, vr0max);
 
   /* Failed to find an efficient meet.  Before giving up and setting
@@ -6162,7 +6133,7 @@  value_range_base::union_helper (const value_range_base *vr0,
       && range_includes_zero_p (vr0) == 0
       && range_includes_zero_p (vr1) == 0)
     {
-      tem.set_nonnull (vr0->type ());
+      tem.set_non_zero (vr0->type ());
       return tem;
     }
 
diff --git a/gcc/tree-vrp.h b/gcc/tree-vrp.h
index 9d52b428d05..11ecf939ee2 100644
--- a/gcc/tree-vrp.h
+++ b/gcc/tree-vrp.h
@@ -46,8 +46,8 @@  public:
 
   void set (value_range_kind, tree, tree);
   void set (tree);
-  void set_nonnull (tree);
-  void set_null (tree);
+  void set_non_zero (tree);
+  void set_zero (tree);
 
   enum value_range_kind kind () const;
   tree min () const;
@@ -72,6 +72,7 @@  public:
   bool may_contain_p (tree) const;
   void set_and_canonicalize (enum value_range_kind, tree, tree);
   bool zero_p () const;
+  bool non_zero_p () const;
   bool singleton_p (tree *result = NULL) const;
   void dump (FILE *) const;
 
@@ -118,8 +119,6 @@  class GTY((user)) value_range : public value_range_base
   /* Deep-copies equiv bitmap argument.  */
   void set (value_range_kind, tree, tree, bitmap = NULL);
   void set (tree);
-  void set_nonnull (tree);
-  void set_null (tree);
 
   bool operator== (const value_range &) const /* = delete */;
   bool operator!= (const value_range &) const /* = delete */;
@@ -222,6 +221,16 @@  value_range_base::zero_p () const
 	  && integer_zerop (m_max));
 }
 
+/* Return TRUE if range is non-zero.  */
+
+inline bool
+value_range_base::non_zero_p () const
+{
+  return (m_kind == VR_ANTI_RANGE
+	  && integer_zerop (m_min)
+	  && integer_zerop (m_max));
+}
+
 extern void dump_value_range (FILE *, const value_range *);
 extern void dump_value_range (FILE *, const value_range_base *);
 
diff --git a/gcc/vr-values.c b/gcc/vr-values.c
index 0e10aca92bb..fa91ee5aade 100644
--- a/gcc/vr-values.c
+++ b/gcc/vr-values.c
@@ -118,7 +118,10 @@  vr_values::get_value_range (const_tree var)
 	  if (POINTER_TYPE_P (TREE_TYPE (sym))
 	      && (nonnull_arg_p (sym)
 		  || get_ptr_nonnull (var)))
-	    vr->set_nonnull (TREE_TYPE (sym));
+	    {
+	      vr->set_non_zero (TREE_TYPE (sym));
+	      vr->equiv_clear ();
+	    }
 	  else if (INTEGRAL_TYPE_P (TREE_TYPE (sym)))
 	    {
 	      get_range_info (var, *vr);
@@ -130,7 +133,10 @@  vr_values::get_value_range (const_tree var)
 	}
       else if (TREE_CODE (sym) == RESULT_DECL
 	       && DECL_BY_REFERENCE (sym))
-	vr->set_nonnull (TREE_TYPE (sym));
+	{
+	  vr->set_non_zero (TREE_TYPE (sym));
+	  vr->equiv_clear ();
+	}
     }
 
   return vr;
@@ -858,7 +864,10 @@  vr_values::extract_range_from_binary_expr (value_range *vr,
 	  || (vr1.kind () == VR_ANTI_RANGE
 	      && vr1.min () == op0
 	      && vr1.min () == vr1.max ())))
-      vr->set_nonnull (expr_type);
+    {
+      vr->set_non_zero (expr_type);
+      vr->equiv_clear ();
+    }
 }
 
 /* Extract range information from a unary expression CODE OP0 based on
@@ -1085,7 +1094,8 @@  vr_values::extract_range_basic (value_range *vr, gimple *stmt)
 	      && TREE_CODE (SSA_NAME_VAR (arg)) == PARM_DECL
 	      && cfun->after_inlining)
 	    {
-	      vr->set_null (type);
+	      vr->set_zero (type);
+	      vr->equiv_clear ();
 	      return;
 	    }
 	  break;
@@ -1392,7 +1402,10 @@  vr_values::extract_range_basic (value_range *vr, gimple *stmt)
       && gimple_stmt_nonnegative_warnv_p (stmt, &sop))
     set_value_range_to_nonnegative (vr, type);
   else if (vrp_stmt_computes_nonzero (stmt))
-    vr->set_nonnull (type);
+    {
+      vr->set_non_zero (type);
+      vr->equiv_clear ();
+    }
   else
     vr->set_varying ();
 }