On Tue, 13 Nov 2018, Aldy Hernandez wrote:
>
> > > The tricky part starts in the prologue for
> > >
> > > if (vr0->undefined_p ())
> > > {
> > > vr0->deep_copy (vr1);
> > > return;
> > > }
> > >
> > > but yes, we probably can factor out a bit more common code
> > > here. I'll see to followup with more minor cleanups this
> > > week (noticed a few details myself).
> >
> > Like this? (untested)
>
> I would inline value_range_base::union_helper into value_range_base::union_,
> and remove all the undefined/varying/etc stuff from value_range::union_.
>
> If should work because upon return from value_range_base::union_, in the
> this->undefined_p case, the base class will copy everything but the
> equivalences. Then the derived union_ only has to nuke the equivalences if
> this->undefined or this->varying, and the equivalences' IOR just works.
>
> For instance, in the case where other->undefined_p, there shouldn't be
> anything in the equivalences so the IOR won't copy anything to this as
> expected. Similarly for this->varying_p.
>
> In the case of other->varying, this will already been set to varying so
> neither this nor other should have anything in their equivalence fields, so
> the IOR won't do anything.
>
> I think I covered all of them...the bitmap math should just work. What do you
> think?
I think the only case that will not work is the case when this->undefined
(when we need the deep copy). Because we'll not get the bitmap from
other in that case. So I've settled with the thing below (just
special-casing that very case)
> Finally, as I've hinted before, I think we need to be careful that any time we
> change state to VARYING / UNDEFINED from a base method, that the derived class
> is in a sane state (there are no equivalences set per the API contract). This
> was not originally enforced in VRP, and I wouldn't be surprised if there are
> dragons if we enforce honesty. I suppose, since we have an API, we could
> enforce this lazily: any time equiv() is called, clear the equivalences or
> return NULL if it's varying or undefined? Just a thought.
I have updated ->update () to adjust equiv when we update to VARYING
or UNDEFINED.
Richard.
Index: gcc/tree-ssanames.c
===================================================================
--- gcc/tree-ssanames.c (revision 266056)
+++ gcc/tree-ssanames.c (working copy)
@@ -401,7 +401,7 @@ set_range_info (tree name, enum value_ra
/* Store range information for NAME from a value_range. */
void
-set_range_info (tree name, const value_range &vr)
+set_range_info (tree name, const value_range_base &vr)
{
wide_int min = wi::to_wide (vr.min ());
wide_int max = wi::to_wide (vr.max ());
@@ -434,7 +434,7 @@ get_range_info (const_tree name, wide_in
in a value_range VR. Returns the value_range_kind. */
enum value_range_kind
-get_range_info (const_tree name, value_range &vr)
+get_range_info (const_tree name, value_range_base &vr)
{
tree min, max;
wide_int wmin, wmax;
Index: gcc/tree-ssanames.h
===================================================================
--- gcc/tree-ssanames.h (revision 266056)
+++ gcc/tree-ssanames.h (working copy)
@@ -69,11 +69,11 @@ struct GTY ((variable_size)) range_info_
/* Sets the value range to SSA. */
extern void set_range_info (tree, enum value_range_kind, const wide_int_ref &,
const wide_int_ref &);
-extern void set_range_info (tree, const value_range &);
+extern void set_range_info (tree, const value_range_base &);
/* Gets the value range from SSA. */
extern enum value_range_kind get_range_info (const_tree, wide_int *,
wide_int *);
-extern enum value_range_kind get_range_info (const_tree, value_range &);
+extern enum value_range_kind get_range_info (const_tree, value_range_base &);
extern void set_nonzero_bits (tree, const wide_int_ref &);
extern wide_int get_nonzero_bits (const_tree);
extern bool ssa_name_has_boolean_range (tree);
Index: gcc/tree-vrp.c
===================================================================
--- gcc/tree-vrp.c (revision 266056)
+++ gcc/tree-vrp.c (working copy)
@@ -6084,125 +6084,73 @@ value_range::intersect (const value_rang
}
}
-/* Meet operation for value ranges. Given two value ranges VR0 and
- VR1, store in VR0 a range that contains both VR0 and VR1. This
- may not be the smallest possible such range. */
-
-void
-value_range_base::union_ (const value_range_base *other)
-{
- if (other->undefined_p ())
- {
- /* this already has the resulting range. */
- return;
- }
+/* Helper for meet operation for value ranges. Given two value ranges VR0 and
+ VR1, return a range that contains both VR0 and VR1. This may not be the
+ smallest possible such range. */
+
+value_range_base
+value_range_base::union_helper (const value_range_base *vr0,
+ const value_range_base *vr1)
+{
+ /* VR0 has the resulting range if VR1 is undefined or VR0 is varying. */
+ if (vr1->undefined_p ()
+ || vr0->varying_p ())
+ return *vr0;
+
+ /* VR1 has the resulting range if VR0 is undefined or VR1 is varying. */
+ if (vr0->undefined_p ()
+ || vr1->varying_p ())
+ return *vr1;
- if (this->undefined_p ())
- {
- *this = *other;
- return;
- }
+ value_range_kind vr0type = vr0->kind ();
+ tree vr0min = vr0->min ();
+ tree vr0max = vr0->max ();
+ union_ranges (&vr0type, &vr0min, &vr0max,
+ vr1->kind (), vr1->min (), vr1->max ());
- if (this->varying_p ())
- {
- /* Nothing to do. VR0 already has the resulting range. */
- return;
- }
+ /* Work on a temporary so we can still use vr0 when union returns varying.
*/
+ value_range tem;
+ tem.set_and_canonicalize (vr0type, vr0min, vr0max);
- if (other->varying_p ())
+ /* 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)
{
- this->set_varying ();
- return;
+ tem.set_nonnull (vr0->type ());
+ return tem;
}
- value_range saved (*this);
- value_range_kind vr0type = this->kind ();
- tree vr0min = this->min ();
- tree vr0max = this->max ();
- union_ranges (&vr0type, &vr0min, &vr0max,
- other->kind (), other->min (), other->max ());
- *this = value_range_base (vr0type, vr0min, vr0max);
- if (this->varying_p ())
- {
- /* 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 (range_includes_zero_p (&saved) == 0
- && range_includes_zero_p (other) == 0)
- {
- tree zero = build_int_cst (saved.type (), 0);
- *this = value_range_base (VR_ANTI_RANGE, zero, zero);
- return;
- }
-
- this->set_varying ();
- return;
- }
- this->set_and_canonicalize (this->kind (), this->min (), this->max ());
+ return tem;
}
+
/* Meet operation for value ranges. Given two value ranges VR0 and
VR1, store in VR0 a range that contains both VR0 and VR1. This
may not be the smallest possible such range. */
void
-value_range::union_helper (value_range *vr0, const value_range *vr1)
+value_range_base::union_ (const value_range_base *other)
{
- if (vr1->undefined_p ())
- {
- /* VR0 already has the resulting range. */
- return;
- }
-
- if (vr0->undefined_p ())
- {
- vr0->deep_copy (vr1);
- return;
- }
-
- if (vr0->varying_p ())
+ if (dump_file && (dump_flags & TDF_DETAILS))
{
- /* Nothing to do. VR0 already has the resulting range. */
- return;
+ fprintf (dump_file, "Meeting\n ");
+ dump_value_range (dump_file, this);
+ fprintf (dump_file, "\nand\n ");
+ dump_value_range (dump_file, other);
+ fprintf (dump_file, "\n");
}
- if (vr1->varying_p ())
- {
- vr0->set_varying ();
- return;
- }
+ *this = union_helper (this, other);
- value_range_kind vr0type = vr0->kind ();
- tree vr0min = vr0->min ();
- tree vr0max = vr0->max ();
- union_ranges (&vr0type, &vr0min, &vr0max,
- vr1->kind (), vr1->min (), vr1->max ());
- /* Work on a temporary so we can still use vr0 when union returns varying.
*/
- value_range tem;
- tem.set_and_canonicalize (vr0type, vr0min, vr0max);
- if (tem.varying_p ())
+ if (dump_file && (dump_flags & TDF_DETAILS))
{
- /* 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 (range_includes_zero_p (vr0) == 0
- && range_includes_zero_p (vr1) == 0)
- {
- vr0->set_nonnull (vr0->type ());
- return;
- }
-
- vr0->set_varying ();
- return;
+ fprintf (dump_file, "to\n ");
+ dump_value_range (dump_file, this);
+ fprintf (dump_file, "\n");
}
- vr0->update (tem.kind (), tem.min (), tem.max ());
-
- /* The resulting set of equivalences is always the intersection of
- the two sets. */
- if (vr0->m_equiv && vr1->m_equiv && vr0->m_equiv != vr1->m_equiv)
- bitmap_and_into (vr0->m_equiv, vr1->m_equiv);
- else if (vr0->m_equiv && !vr1->m_equiv)
- bitmap_clear (vr0->m_equiv);
}
void
@@ -6216,7 +6164,24 @@ value_range::union_ (const value_range *
dump_value_range (dump_file, other);
fprintf (dump_file, "\n");
}
- union_helper (this, other);
+
+ /* If THIS is undefined we want to pick up equivalences from OTHER.
+ Just special-case this here rather than trying to fixup after the fact.
*/
+ if (this->undefined_p ())
+ this->deep_copy (other);
+ else
+ {
+ value_range_base tem = union_helper (this, other);
+ this->update (tem.kind (), tem.min (), tem.max ());
+
+ /* The resulting set of equivalences is always the intersection of
+ the two sets. */
+ if (this->m_equiv && other->m_equiv && this->m_equiv != other->m_equiv)
+ bitmap_and_into (this->m_equiv, other->m_equiv);
+ else if (this->m_equiv && !other->m_equiv)
+ bitmap_clear (this->m_equiv);
+ }
+
if (dump_file && (dump_flags & TDF_DETAILS))
{
fprintf (dump_file, "to\n ");
@@ -6867,11 +6832,11 @@ make_pass_vrp (gcc::context *ctxt)
/* Worker for determine_value_range. */
static void
-determine_value_range_1 (value_range *vr, tree expr)
+determine_value_range_1 (value_range_base *vr, tree expr)
{
if (BINARY_CLASS_P (expr))
{
- value_range vr0, vr1;
+ value_range_base vr0, vr1;
determine_value_range_1 (&vr0, TREE_OPERAND (expr, 0));
determine_value_range_1 (&vr1, TREE_OPERAND (expr, 1));
extract_range_from_binary_expr (vr, TREE_CODE (expr), TREE_TYPE (expr),
@@ -6879,7 +6844,7 @@ determine_value_range_1 (value_range *vr
}
else if (UNARY_CLASS_P (expr))
{
- value_range vr0;
+ value_range_base vr0;
determine_value_range_1 (&vr0, TREE_OPERAND (expr, 0));
extract_range_from_unary_expr (vr, TREE_CODE (expr), TREE_TYPE (expr),
&vr0, TREE_TYPE (TREE_OPERAND (expr, 0)));
@@ -6908,7 +6873,7 @@ determine_value_range_1 (value_range *vr
value_range_kind
determine_value_range (tree expr, wide_int *min, wide_int *max)
{
- value_range vr;
+ value_range_base vr;
determine_value_range_1 (&vr, expr);
if (vr.constant_p ())
{
Index: gcc/tree-vrp.h
===================================================================
--- gcc/tree-vrp.h (revision 266056)
+++ gcc/tree-vrp.h (working copy)
@@ -77,6 +77,8 @@ public:
protected:
void check ();
+ static value_range_base union_helper (const value_range_base *,
+ const value_range_base *);
enum value_range_kind m_kind;
@@ -145,7 +147,6 @@ class GTY((user)) value_range : public v
void check ();
bool equal_p (const value_range &, bool ignore_equivs) const;
void intersect_helper (value_range *, const value_range *);
- void union_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. */
