On 10/4/19 1:17 PM, Jeff Law wrote:
On 10/4/19 10:14 AM, Aldy Hernandez wrote:
On 10/4/19 12:02 PM, Jeff Law wrote:
On 10/4/19 9:49 AM, Aldy Hernandez wrote:
On 10/4/19 11:38 AM, Jeff Law wrote:
On 10/4/19 6:59 AM, Aldy Hernandez wrote:
When I did the value_range canonicalization work, I noticed that an
unsigned [1,MAX] and an ~[0,0] could be two different representations
for the same thing. I didn't address the problem then because callers
to ranges_from_anti_range() would go into an infinite loop trying to
extract ~[0,0] into [1,MAX] and []. We had a lot of callers to
ranges_from_anti_range, and it smelled like a rat's nest, so I bailed.
Now that we have one main caller (from the symbolic PLUS/MINUS
handling), it's a lot easier to contain. Well, singleton_p also calls
it, but it's already handling nonzero specially, so it wouldn't be affected.
With some upcoming cleanups I'm about to post, the fact that
[1,MAX] and
~[0,0] are equal_p(), but not nonzero_p(), matters. Plus, it's just
good form to have one representation, giving us the ability to pick at
nonzero_p ranges with ease.
The code in extract_range_from_plus_minus_expr() continues to be a
mess
(as it has always been), but at least it's contained, and with this
patch, it's slightly smaller.
Note, I'm avoiding adding a comment header for functions with highly
descriptive obvious names.
OK?
Aldy
canonicalize-nonzero-ranges.patch
commit 1c333730deeb4ddadc46ad6d12d5344f92c0352c
Author: Aldy Hernandez <al...@redhat.com>
Date: Fri Oct 4 08:51:25 2019 +0200
Canonicalize UNSIGNED [1,MAX] into ~[0,0].
Adapt PLUS/MINUS symbolic handling, so it doesn't call
ranges_from_anti_range with a VR_ANTI_RANGE containing one
sub-range.
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 6e4f145af46..3934b41fdf9 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,18 @@
+2019-10-04 Aldy Hernandez <al...@redhat.com>
+
+ * tree-vrp.c (value_range_base::singleton_p): Use num_pairs
+ instead of calling vrp_val_is_*.
+ (value_range_base::set): Canonicalize unsigned [1,MAX] into
+ non-zero.
+ (range_has_numeric_bounds_p): New.
+ (range_int_cst_p): Use range_has_numeric_bounds_p.
+ (ranges_from_anti_range): Assert that we won't recurse
+ indefinitely.
+ (extract_extremes_from_range): New.
+ (extract_range_from_plus_minus_expr): Adapt so we don't call
+ ranges_from_anti_range with an anti-range containing only one
+ sub-range.
So no problem with the implementation, but I do have a higher level
question.
One of the goals of the representation side of the Ranger project is to
drop anti-ranges. Canonicalizing [1, MAX] to ~[0,0] seems to be going
in the opposite direction. So do we really want to canonicalize to
~[0,0]?
Hmmm, Andrew had the same question.
It really doesn't matter what we canonicalize too, as long as we're
consistent, but there are a bunch of non-zero tests throughout that were
checking for the ~[0,0] construct, and I didn't want to rock the boat
too much. Although in all honesty, most of those should already be
converted to the ::nonzero_p() API.
However, if we canonicalize into [1,MAX] for unsigned, we have the
problem that a signed non-zero will still be ~[0,0], so our ::nonzero_p
code will have to check two different representations, not to mention it
will now have to check TYPE_UNSIGNED(type).
ISTM that the right thing to do is to first move to the ::nonzero_p API,
which should be a behavior preserving change. It'd probably be a medium
sized change, but highly mechanical and behavior preserving, so easy to
review.
Ughh, please no? This was just a means to get the general range_fold*
cleanups which are important and make everything easier to read. I'd
rather not get distracted by having to audit all the nonzero checking
throughout.
Doesn't the audit just have to look for an open-coded check for ~[0,0]
and convert any to use the API? I don't think we have *that* many
anti-range bits I wouldn't think this would be terrible.
What am I missing here that makes this so painful?
I think I'm just suffering from PTSD from anything VRP related. If you
fix something correctly, 20 things are bound to break because they were
expecting incorrect behavior.
Besides, we can't get away from anti-ranges inasmuch as we have
value_range_base, which hopefully we can move away from in a future
release. So this will eventually become a non-issue. At that point,
we'll loose ALL anti-ranges once and for all.
Even if we can't get away from them, introducing more, particularly
canonicalizing on a form using anti-ranges just seems like we're going
backwards.
If we funnel everything through the established API, then changing the
canonical form becomes trivial because it's isolated to just two places.
The test ::nonzero_p method and the canonicalization point.
~[0,0] has been the accepted way for a long time, I'd really prefer to
keep that (for now).
It has. Very true. But I don't necessarily think that means we should
be introducing even more of 'em.
And really, this is just plain ugly:
bool
value_range_base::nonzero_p ()
{
if (m_kind == VR_ANTI_RANGE
&& !TYPE_UNSIGNED (type ())
&& integer_zerop (m_min)
&& integer_zerop (m_max))
return true;
return (m_kind == VR_RANGE
&& TYPE_UNSIGNED (type ())
&& integer_onep (m_min)
&& vrp_val_is_max (m_max));
}
That doesn't seem bad at all, particularly if this is where we contain
the wart.
Fair enough. I guess I don't care what we settle on, inasmuch as we
canonicalize into one true value. For some reason, I thought the above
nonzero would cause you to cringe, I guess not :).
Happily, normalizing into ~0 for signed and [1,MAX] for unsigned,
simplified the patch because it on longer needs tweaks to
ranges_from_anti_range.
OK for trunk?
Aldy
p.s. This still leaves open the issue with ipa_vr's handling of
nonzero_p. As per my last message, I've adjusted it to check for either
~[0,0] or the [1,MAX] variant for unsigned, since before this patch
there were two ways of representing the same thing. However, ipa-prop
has no API (it open codes everything), as it has rolled its own version
of "value ranges" with wide-ints.
class GTY(()) ipa_vr
{
public:
/* The data fields below are valid only if known is true. */
bool known;
enum value_range_kind type;
wide_int min;
wide_int max;
bool nonzero_p (tree) const;
};
I'm tempted to leave the nonzero_p which checks for both ~0 and [1,MAX]:
bool
ipa_vr::nonzero_p (tree expr_type) const
{
if (type == VR_ANTI_RANGE && wi::eq_p (min, 0) && wi::eq_p (max, 0))
return true;
unsigned prec = TYPE_PRECISION (expr_type);
return (type == VR_RANGE
&& TYPE_UNSIGNED (expr_type)
&& wi::eq_p (min, wi::one (prec))
&& wi::eq_p (max, wi::max_value (prec, TYPE_SIGN (expr_type))));
}
I don't care either way.
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/evrp4.c b/gcc/testsuite/gcc.dg/tree-ssa/evrp4.c
index ebb87ed38b0..ba2f6b9b430 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/evrp4.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/evrp4.c
@@ -17,4 +17,4 @@ int bar (struct st *s)
foo (&s->a);
}
-/* { dg-final { scan-tree-dump "\~\\\[0B, 0B\\\]" "evrp" } } */
+/* { dg-final { scan-tree-dump "\\\[1B, -1B\\\]" "evrp" } } */
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index d69cfb107cb..cffa0508340 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -800,13 +800,13 @@ value_range_base::set (enum value_range_kind kind, tree min, tree max)
kind = VR_RANGE;
}
else if (is_min
- /* As a special exception preserve non-null ranges. */
- && !(TYPE_UNSIGNED (TREE_TYPE (min))
- && integer_zerop (max)))
+ /* Allow non-zero pointers to be normalized to [1,MAX]. */
+ || (POINTER_TYPE_P (TREE_TYPE (min))
+ && integer_zerop (min)))
{
tree one = build_int_cst (TREE_TYPE (max), 1);
min = int_const_binop (PLUS_EXPR, max, one);
- max = vrp_val_max (TREE_TYPE (max));
+ max = vrp_val_max (TREE_TYPE (max), true);
kind = VR_RANGE;
}
else if (is_max)
diff --git a/gcc/tree-vrp.h b/gcc/tree-vrp.h
index a3f9e90699d..4bfdfeb8f79 100644
--- a/gcc/tree-vrp.h
+++ b/gcc/tree-vrp.h
@@ -245,16 +245,6 @@ value_range_base::zero_p () const
&& integer_zerop (m_max));
}
-/* Return TRUE if range is nonzero. */
-
-inline bool
-value_range_base::nonzero_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 *);
@@ -322,6 +312,23 @@ 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 range is nonzero. */
+
+inline bool
+value_range_base::nonzero_p () const
+{
+ if (m_kind == VR_ANTI_RANGE
+ && !TYPE_UNSIGNED (type ())
+ && integer_zerop (m_min)
+ && integer_zerop (m_max))
+ return true;
+
+ return (m_kind == VR_RANGE
+ && TYPE_UNSIGNED (type ())
+ && integer_onep (m_min)
+ && vrp_val_is_max (m_max, true));
+}
+
/* Return TRUE if *VR includes the value zero. */
inline bool
