On Tue, 13 Nov 2018, Richard Biener wrote:
> On Mon, 12 Nov 2018, Aldy Hernandez wrote:
>
> > On 11/12/18 7:12 AM, Richard Biener wrote:
> > >
> > > This mainly tries to rectify the workaround I put in place for ipa-cp.c
> > > needing to build value_range instead of value_range_base for calling
> > > extract_range_from_unary_expr.
> > >
> > > To make this easier I moved more set_* functions to methods.
> > >
> > > Then for some reason I chose to fix the rathole of equiv bitmap sharing
> > > after finding at least one real bug
> >
> > By the way, I've seen that the equiv_add() calls in vr-values.c sometimes
> > set
> > equivalences for VARYING and UNDEFINED which in theory shouldn't happen.
> > I've
> > been too chicken to follow that hole.
> >
> > I think we should assert everywhere that we set the equivalences, that we're
> > not talking about VARYING or UNDEFINED.
> >
> > > @@ -6168,37 +6172,30 @@ value_range::union_helper (value_range *vr0, const
> > > value_range *vr1)
> > > return;
> > > }
> > > - value_range saved (*vr0);
> > > 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 ());
> > > - *vr0 = value_range (vr0type, vr0min, vr0max);
> > > - if (vr0->varying_p ())
> > > + /* 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 ())
> >
> > I'm not a big fan of the code duplication in the union chunks. You're
> > adding
> > more places that need to be kept in sync.
> >
> > I think value_range::union_ could be easily coded as:
> >
> > value_range_base::union_ (other);
> > union_helper (this, other);
> > if (flag_checking)
> > check ();
> >
> > And have union_helper only deal with the equivalence stuff.
>
> 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)
Richard.
Index: gcc/tree-vrp.c
===================================================================
--- gcc/tree-vrp.c (revision 266056)
+++ gcc/tree-vrp.c (working copy)
@@ -6084,6 +6084,39 @@ value_range::intersect (const value_rang
}
}
+/* 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. */
+
+static value_range_base
+value_range_base::union_helper (const value_range_base *vr0,
+ const value_range_base *vr1)
+{
+ 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);
+
+ /* 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)
+ {
+ tem.set_nonnull (vr0->type ());
+ return tem;
+ }
+
+ 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. */
@@ -6115,108 +6148,55 @@ value_range_base::union_ (const value_ra
return;
}
- 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 ());
+ *this = union_helper (this, other);
}
-/* 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::union_ (const value_range *other)
{
- if (vr1->undefined_p ())
+ if (dump_file && (dump_flags & TDF_DETAILS))
{
- /* 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 (vr0->undefined_p ())
+ if (other->undefined_p ())
{
- vr0->deep_copy (vr1);
+ /* VR0 already has the resulting range. */
return;
}
- if (vr0->varying_p ())
+ if (this->undefined_p ())
{
- /* Nothing to do. VR0 already has the resulting range. */
+ this->deep_copy (other);
return;
}
- if (vr1->varying_p ())
+ if (this->varying_p ())
{
- vr0->set_varying ();
+ /* Nothing to do. VR0 already has the resulting range. */
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 ());
- /* 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 (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 (range_includes_zero_p (vr0) == 0
- && range_includes_zero_p (vr1) == 0)
- {
- vr0->set_nonnull (vr0->type ());
- return;
- }
-
- vr0->set_varying ();
+ this->set_varying ();
return;
}
- vr0->update (tem.kind (), tem.min (), tem.max ());
+
+ 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 (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);
-}
+ 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);
-void
-value_range::union_ (const value_range *other)
-{
- if (dump_file && (dump_flags & TDF_DETAILS))
- {
- 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");
- }
- union_helper (this, other);
if (dump_file && (dump_flags & TDF_DETAILS))
{
fprintf (dump_file, "to\n ");
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. */
