On Fri, May 31, 2019 at 2:27 AM Jeff Law <l...@redhat.com> wrote:
>
> On 5/29/19 10:20 AM, Aldy Hernandez wrote:
> > On 5/29/19 12:12 PM, Jeff Law wrote:
> >> On 5/29/19 9:58 AM, Aldy Hernandez wrote:
> >>> 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?
> >> ISTM that if we have an implementation constraint that says a VR_VARYING
> >> or VR_UNDEFINED range can't have equivalences, then we need to honor
> >> that at the minimum for anything returned by an external API.  Returning
> >> an inconsistent state is bad.  I'd even state that we should try damn
> >> hard to avoid it in internal APIs as well.
> >
> > Agreed * 2.
> >
> >>
> >>>
> >>> 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.  */
> >> Where/when does this happen?
> >
> > The above snippet is not currently in mainline.  It's in the patch I'm
> > proposing to clean up intersect.  It's just that while cleaning up
> > intersect I noticed that if we keep to the value_range API, we end up
> > clobbering an equivalence to a VR_UNDEFINED that we depend up further up
> > the call chain.
> >
> > The reason it doesn't happen in mainline is because intersect_helper
> > bails early on an undefined, thus leaving the problematic equivalence
> > intact.
> >
> > You can see it in mainline though, with the following testcase:
> >
> > int f(int x)
> > {
> >   if (x != 0 && x != 1)
> >     return -2;
> >
> >   return !x;
> > }
> >
> > Break in evrp_range_analyzer::record_ranges_from_stmt() and see that the
> > call to extract_range_from_stmt() returns a VR of undefined *WITH*
> > equivalences:
> >
> >       vr_values->extract_range_from_stmt (stmt, &taken_edge, &output, &vr);
> >
> > This VR is later fed to update_value_range() and ultimately intersect(),
> > which in mainline, leaves the equivalences alone, but IMO should respect
> > that API and nuke them.
> So I think this is coming from extract_range_from_ssa_name:
>
> > void
> > vr_values::extract_range_from_ssa_name (value_range *vr, tree var)
> > {
> >   value_range *var_vr = get_value_range (var);
> >
> >   if (!var_vr->varying_p ())
> >     vr->deep_copy (var_vr);
> >   else
> >     vr->set (var);
> >
> >   vr->equiv_add (var, get_value_range (var), &vrp_equiv_obstack);
> > }
>
> Where we blindly add VAR to the equiv bitmap in the range.
>
> AFAICT gcc-7 has equivalent code, it's just not inside the class.
>
> I don't know what the fallout might be, but we could conditionalize the
> call to add_equivalence to avoid the inconsistent state.

Well, the issue is that the equivalence _is_ there.  So above we
know that we never end up with VARYING but the deep_copy
can bring over UNDEFINED to us.  I guess we _could_ elide
the equiv adding then.  OTOH the equivalence does look
useful for predicate simplification which IIRC doesn't treat
UNDEFINED != x in a useful way.  Which would mean allowing
equivalences on UNDEFINED.  That somewhat makes sense
since UNDEFINED is bottom-most in the lattice and one up
(CONSTANT) we have equivalences while just on topmost
(VARYING) we do not.

That said, I never liked equivalences - they are an odd way
to represent ranges intersected from multiple ranges thus
a way out of our too simplistic range representation.

So lets fix this in a way avoiding testsuite fallout.  But please
not with special hacks in consumers.  Does guariding the
above equiv_add with !vr->undefined_p () fix things?

Richard.

> Jeff

Reply via email to