Re: value_range_base::{non_zero_p, set_zero, set_non_zero}
Thanks. I will adjust accordingly. On Fri, May 31, 2019, 02:26 Martin Sebor 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 > >
Re: value_range_base::{non_zero_p, set_zero, set_non_zero}
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.) -- Joseph S. Myers jos...@codesourcery.com
Re: value_range_base::{non_zero_p, set_zero, set_non_zero}
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
Re: value_range_base::{non_zero_p, set_zero, set_non_zero}
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 > 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
value_range_base::{non_zero_p, set_zero, set_non_zero}
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
commit 55294d340a0727dbe985ee4bf3c1969a19bcbe6d
Author: Aldy Hernandez
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 ())
+ 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 () && !range_includes_zero_p ())
- vr->set_nonnull (expr_type);
- else if (range_is_null () && range_is_null ())
- vr->set_null (expr_type);
+ vr->set_non_zero (expr_type);
+ else if (vr0.zero_p () && vr1.zero_p ())
+ vr->set_zero (expr_type);
