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. */