On 5/29/19 9:24 AM, Richard Biener wrote:
On Wed, May 29, 2019 at 2:18 PM Aldy Hernandez <al...@redhat.com> wrote:
As per the API, and the original documentation to value_range,
VR_UNDEFINED and VR_VARYING should never have equivalences. However,
equiv_add is tacking on equivalences blindly, and there are various
regressions that happen if I fix this oversight.
void
value_range::equiv_add (const_tree var,
const value_range *var_vr,
bitmap_obstack *obstack)
{
if (!m_equiv)
m_equiv = BITMAP_ALLOC (obstack);
unsigned ver = SSA_NAME_VERSION (var);
bitmap_set_bit (m_equiv, ver);
if (var_vr && var_vr->m_equiv)
bitmap_ior_into (m_equiv, var_vr->m_equiv);
}
Is this a bug in the documentation / API, or is equiv_add incorrect and
we should fix the fall-out elsewhere?
I think this must have been crept in during the classification. If you
go back to say GCC 7 you shouldn't see value-ranges with
UNDEFINED/VARYING state in the lattice that have equivalences.
It may not be easy to avoid with the new classy interface but we're
certainly not tacking on them "blindly". At least we're not supposed
to. As usual the intermediate state might be "broken" but
intermediateness is not sth the new class "likes".
It looks like extract_range_from_stmt (by virtue of
vrp_visit_assignment_or_call and then extract_range_from_ssa_name)
returns one of these intermediate ranges. It would seem to me that an
outward looking API method like vr_values::extract_range_from_stmt
shouldn't be returning inconsistent ranges. Or are there no guarantees
for value_ranges from within all of vr_values?
Perhaps I should give a little background. As part of your
value_range_base re-factoring last year, you mentioned that you didn't
split out intersect like you did union because of time or oversight. I
have code to implement intersect (attached), for which I've noticed that
I must leave equivalences intact, even when transitioning to VR_UNDEFINED:
[from the attached patch]
+ /* If THIS is varying we want to pick up equivalences from OTHER.
+ Just special-case this here rather than trying to fixup after the
+ fact. */
+ if (this->varying_p ())
+ this->deep_copy (other);
+ else if (this->undefined_p ())
+ /* ?? Leave any equivalences already present in an undefined.
+ This is technically not allowed, but we may get an in-flight
+ value_range in an intermediate state. */
+ ;
What is happening is that in evrp's record_ranges_from_stmt, we call
extract_range_from_stmt which returns an inconsistent VR_UNDEFINED with
an equivalence, which is then fed to update_value_range() and finally
value_range::intersect. The VR_UNDEFINED equivalence must not be
removed in the intersect, because update_value_range() will get confused
as to whether this is a new VR or not (because VR_UNDEFINED with no
equivalences is not the same as VR_UNDEFINED with equivalences-- see
"is_new" in update_value_range).
I'd rather not special case VR_UNDEFINED in the intersect code as above,
but if you think extract_range_from_stmt() can return an "intermediate"
range, and thus intersect must handle them too, then I suppose we could
leave this in.
What do you suggest?
Oh yeah, is the attached patch OK for trunk? I can post in a separate
thread if you prefer, but thought it relevant here :).
Aldy
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 9f194540327..9494520ba33 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,19 @@
+2019-05-29 Aldy Hernandez <al...@redhat.com>
+
+ * tree-vrp.h (value_range_base::intersect): New.
+ (value_range::intersect_helper): Move from here...
+ (value_range_base::intersect_helper): ...to here.
+ * tree-vrp.c (value_range::intersect_helper): Rename to...
+ (value_range_base::intersect_helper): ...this, and rewrite to
+ return a value instead of modifying THIS in place.
+ Also, move equivalence handling...
+ (value_range::intersect): ...here, while calling intersect_helper.
+ * gimple-fold.c (size_must_be_zero_p): Use value_range_base when
+ calling intersect.
+ * gimple-ssa-evrp-analyze.c (ecord_ranges_from_incoming_edge):
+ Same.
+ * vr-values.c (vrp_evaluate_conditional_warnv_with_ops): Same.
+
2019-05-29 Aldy Hernandez <al...@redhat.com>
* tree-vrp.h (value_range_base::non_zero_p): New.
* tree-vrp.c (range_is_null): Remove.
diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index b3e931744f8..8b8331eb555 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -684,10 +684,10 @@ size_must_be_zero_p (tree size)
/* Compute the value of SSIZE_MAX, the largest positive value that
can be stored in ssize_t, the signed counterpart of size_t. */
wide_int ssize_max = wi::lshift (wi::one (prec), prec - 1) - 1;
- value_range valid_range (VR_RANGE,
- build_int_cst (type, 0),
- wide_int_to_tree (type, ssize_max));
- value_range vr;
+ value_range_base valid_range (VR_RANGE,
+ build_int_cst (type, 0),
+ wide_int_to_tree (type, ssize_max));
+ value_range_base vr;
get_range_info (size, vr);
vr.intersect (&valid_range);
return vr.zero_p ();
diff --git a/gcc/gimple-ssa-evrp-analyze.c b/gcc/gimple-ssa-evrp-analyze.c
index bb4e2d6e798..4c68af847e1 100644
--- a/gcc/gimple-ssa-evrp-analyze.c
+++ b/gcc/gimple-ssa-evrp-analyze.c
@@ -210,9 +210,10 @@ evrp_range_analyzer::record_ranges_from_incoming_edge (basic_block bb)
getting first [64, +INF] and then ~[0, 0] from
conditions like (s & 0x3cc0) == 0). */
value_range *old_vr = get_value_range (vrs[i].first);
- value_range tem (old_vr->kind (), old_vr->min (), old_vr->max ());
+ value_range_base tem (old_vr->kind (), old_vr->min (),
+ old_vr->max ());
tem.intersect (vrs[i].second);
- if (tem.equal_p (*old_vr, /*ignore_equivs=*/true))
+ if (tem.equal_p (*old_vr))
continue;
push_value_range (vrs[i].first, vrs[i].second);
if (is_fallthru
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index d65dbcfaeee..2a5103c38aa 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -6033,30 +6033,26 @@ intersect_ranges (enum value_range_kind *vr0type,
}
-/* Intersect the two value-ranges *VR0 and *VR1 and store the result
- in *VR0. This may not be the smallest possible such range. */
+/* Helper for the intersection operation for value ranges. Given two
+ value ranges VR0 and VR1, return the intersection of the two
+ ranges. This may not be the smallest possible such range. */
-void
-value_range::intersect_helper (value_range *vr0, const value_range *vr1)
+value_range_base
+value_range_base::intersect_helper (const value_range_base *vr0,
+ const value_range_base *vr1)
{
/* If either range is VR_VARYING the other one wins. */
if (vr1->varying_p ())
- return;
+ return *vr0;
if (vr0->varying_p ())
- {
- vr0->deep_copy (vr1);
- return;
- }
+ return *vr1;
/* When either range is VR_UNDEFINED the resulting range is
VR_UNDEFINED, too. */
if (vr0->undefined_p ())
- return;
+ return *vr0;
if (vr1->undefined_p ())
- {
- vr0->set_undefined ();
- return;
- }
+ return *vr1;
value_range_kind vr0type = vr0->kind ();
tree vr0min = vr0->min ();
@@ -6066,28 +6062,34 @@ value_range::intersect_helper (value_range *vr0, const value_range *vr1)
/* Make sure to canonicalize the result though as the inversion of a
VR_RANGE can still be a VR_RANGE. Work on a temporary so we can
fall back to vr0 when this turns things to varying. */
- value_range tem;
+ value_range_base tem;
tem.set_and_canonicalize (vr0type, vr0min, vr0max);
/* If that failed, use the saved original VR0. */
if (tem.varying_p ())
- return;
- vr0->update (tem.kind (), tem.min (), tem.max ());
+ return *vr0;
- /* If the result is VR_UNDEFINED there is no need to mess with
- the equivalencies. */
- if (vr0->undefined_p ())
- return;
+ return tem;
+}
+
+void
+value_range_base::intersect (const value_range_base *other)
+{
+ if (dump_file && (dump_flags & TDF_DETAILS))
+ {
+ fprintf (dump_file, "Intersecting\n ");
+ dump_value_range (dump_file, this);
+ fprintf (dump_file, "\nand\n ");
+ dump_value_range (dump_file, other);
+ fprintf (dump_file, "\n");
+ }
- /* The resulting set of equivalences for range intersection is the union of
- the two sets. */
- if (vr0->m_equiv && vr1->m_equiv && vr0->m_equiv != vr1->m_equiv)
- bitmap_ior_into (vr0->m_equiv, vr1->m_equiv);
- else if (vr1->m_equiv && !vr0->m_equiv)
+ *this = intersect_helper (this, other);
+
+ if (dump_file && (dump_flags & TDF_DETAILS))
{
- /* All equivalence bitmaps are allocated from the same obstack. So
- we can use the obstack associated with VR to allocate vr0->equiv. */
- vr0->m_equiv = BITMAP_ALLOC (vr1->m_equiv->obstack);
- bitmap_copy (m_equiv, vr1->m_equiv);
+ fprintf (dump_file, "to\n ");
+ dump_value_range (dump_file, this);
+ fprintf (dump_file, "\n");
}
}
@@ -6102,7 +6104,41 @@ value_range::intersect (const value_range *other)
dump_value_range (dump_file, other);
fprintf (dump_file, "\n");
}
- intersect_helper (this, other);
+
+ /* If THIS is varying we want to pick up equivalences from OTHER.
+ Just special-case this here rather than trying to fixup after the
+ fact. */
+ if (this->varying_p ())
+ this->deep_copy (other);
+ else if (this->undefined_p ())
+ /* ?? Leave any equivalences already present in an undefined.
+ This is technically not allowed, but we may get an in-flight
+ value_range in an intermediate state. */
+ ;
+ else
+ {
+ value_range_base tem = intersect_helper (this, other);
+ this->update (tem.kind (), tem.min (), tem.max ());
+
+ /* If the result is VR_UNDEFINED there is no need to mess with
+ equivalencies. */
+ if (!undefined_p ())
+ {
+ /* The resulting set of equivalences for range intersection
+ is the union of the two sets. */
+ if (m_equiv && other->m_equiv && m_equiv != other->m_equiv)
+ bitmap_ior_into (m_equiv, other->m_equiv);
+ else if (other->m_equiv && !m_equiv)
+ {
+ /* All equivalence bitmaps are allocated from the same
+ obstack. So we can use the obstack associated with
+ VR to allocate this->m_equiv. */
+ m_equiv = BITMAP_ALLOC (other->m_equiv->obstack);
+ bitmap_copy (m_equiv, other->m_equiv);
+ }
+ }
+ }
+
if (dump_file && (dump_flags & TDF_DETAILS))
{
fprintf (dump_file, "to\n ");
diff --git a/gcc/tree-vrp.h b/gcc/tree-vrp.h
index 9ae5e9514c6..945abe992d3 100644
--- a/gcc/tree-vrp.h
+++ b/gcc/tree-vrp.h
@@ -62,6 +62,7 @@ public:
void set_undefined ();
void union_ (const value_range_base *);
+ void intersect (const value_range_base *);
bool operator== (const value_range_base &) const /* = delete */;
bool operator!= (const value_range_base &) const /* = delete */;
@@ -80,6 +81,8 @@ protected:
void check ();
static value_range_base union_helper (const value_range_base *,
const value_range_base *);
+ static value_range_base intersect_helper (const value_range_base *,
+ const value_range_base *);
enum value_range_kind m_kind;
@@ -146,7 +149,6 @@ class GTY((user)) value_range : public value_range_base
/* Deep-copies bitmap argument. */
void set_equiv (bitmap);
void check ();
- void intersect_helper (value_range *, const value_range *);
/* Set of SSA names whose value ranges are equivalent to this one.
This set is only valid when TYPE is VR_RANGE or VR_ANTI_RANGE. */
diff --git a/gcc/vr-values.c b/gcc/vr-values.c
index 0e10aca92bb..41fd9cc7712 100644
--- a/gcc/vr-values.c
+++ b/gcc/vr-values.c
@@ -2343,7 +2343,7 @@ vr_values::vrp_evaluate_conditional_warnv_with_ops (enum tree_code code,
}
else
{
- value_range vro, vri;
+ value_range_base vro, vri;
if (code == GT_EXPR || code == GE_EXPR)
{
vro.set (VR_ANTI_RANGE, TYPE_MIN_VALUE (TREE_TYPE (op0)), x);
