[patch] new API for value_range
Aldy Hernandez
aldyh@redhat.com
Sun Oct 21 17:48:00 GMT 2018
Is this fixed by Richard's patch to 87640? if so, perhaps this is a
duplicate of said PR.
On Sun, Oct 21, 2018 at 3:34 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Wed, Oct 17, 2018 at 7:39 AM Aldy Hernandez <aldyh@redhat.com> wrote:
> >
> >
> >
> > On 10/17/18 6:50 AM, Richard Biener wrote:
> > > On Thu, Oct 11, 2018 at 8:25 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> > >>
> > >>
> > >>
> > >> On 10/11/18 5:47 AM, Richard Biener wrote:
> > >>> On Thu, Oct 11, 2018 at 10:19 AM Aldy Hernandez <aldyh@redhat.com> wrote:
> > >>>>
> > >>>> Hi Richard. Thanks for reviewing.
> > >>>>
> > >>>> On 10/10/18 6:27 AM, Richard Biener wrote:
> > >>>>> On Tue, Oct 9, 2018 at 6:23 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> > >>>>>>
> > >>>>>> I'm assuming the silence on the RFC means nobody is viscerally opposed
> > >>>>>> to it, so here goes the actual implementation ;-).
> > >>>>>>
> > >>>>>> FWI: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00157.html
> > >>>>>>
> > >>>>>> My aim is no change to the current functionality, but there are some
> > >>>>>> things that changed slightly (with no appreciable change in
> > >>>>>> bootstrapability or tests).
> > >>>>>>
> > >>>>>> 1. Primarily, we were building value_ranges by modifying them in-flight
> > >>>>>> with no regards to the validity of the resulting range. By enforcing
> > >>>>>> the API, I noticed we periodically built VR_VARYING / VR_UNDEFINED, but
> > >>>>>> left the equivalence bits uncleared. This comment in the original
> > >>>>>> header file indicates that this is invalid behavior:
> > >>>>>>
> > >>>>>> /* 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. */
> > >>>>>>
> > >>>>>> The API now enforces this upon construction.
> > >>>>>>
> > >>>>>> 2. I also saw us setting min/max when VARYING or UNDEFINED was set.
> > >>>>>> This is invalid. Although these values were being ignored, the API now
> > >>>>>> enforces this.
> > >>>>>>
> > >>>>>> 3. I saw one case in set_value_range_with_overflow() were we were
> > >>>>>> building an invalid range with swapped ranges, where we were silently
> > >>>>>> depending on somebody further up the call chain to swap them for us.
> > >>>>>> I've fixed this at creation.
> > >>>>>>
> > >>>>>> 4. There is one assert in ipcp_vr_lattice which I hope to remove, but
> > >>>>>> left as proof that the original VR_UNDEFINED set was not necessary, as
> > >>>>>> it is now done by default on an empty constructor:
> > >>>>>>
> > >>>>>> - void init () { m_vr.type = VR_UNDEFINED; }
> > >>>>>> + void init () { gcc_assert (m_vr.undefined_p ()); }
> > >>>>>>
> > >>>>>> One last note. The file tree-vrp.c already has a cripple API of sorts
> > >>>>>> in the form of functions (set_value_range_to_varying, etc). I have
> > >>>>>> tried to keep those functions available, by calling the API under the
> > >>>>>> covers, but would be okay in removing them altogether as a follow-up.
> > >>>>>>
> > >>>>>> Please refer to the RFC wrt the min/max/vrtype accessors, as well as the
> > >>>>>> new tree type field.
> > >>>>>>
> > >>>>>> I am quoting the class declaration below to make it easy to review at a
> > >>>>>> high level.
> > >>>>>>
> > >>>>>> Tested on x86-64 Linux. All languages, including Ada and Go.
> > >>>>>>
> > >>>>>> OK for trunk?
> > >>>>>
> > >>>>> Reviewing in patch order.
> > >>>>>
> > >>>>>> Aldy
> > >>>>>>
> > >>>>>> class GTY((for_user)) value_range
> > >>>>>> {
> > >>>>>> public:
> > >>>>>> value_range ();
> > >>>>>> value_range (tree type);
> > >>>>>> value_range (value_range_type, tree type, tree, tree, bitmap = NULL);
> > >>>>>> bool operator== (const value_range &) const;
> > >>>>>> bool operator!= (const value_range &) const;
> > >>>>>> void intersect (const value_range *);
> > >>>>>> void union_ (const value_range *);
> > >>>>>
> > >>>>> with trailing underscore? seriously?
> > >>>>
> > >>>> Hey! You complained about Union() last year, at which point the
> > >>>> consensus was that trailing underscores would be ok for symbol names
> > >>>> that clashed with keywords.
> > >>>
> > >>> ;)
> > >>>
> > >>> I also thought about union_into / union_with. As opposed to a hypothetical
> > >>>
> > >>> value_range union (const value_range& a, const value_range& b)
> > >>>
> > >>> function.
> > >>>
> > >>>> And yes, it was also discussed whether we should overload | and ^ for
> > >>>> union and intersection, but was denied for readability and what have yous.
> > >>>>
> > >>>>>
> > >>>>>> /* Like operator== but ignore equivalence bitmap. */
> > >>>>>> bool ignore_equivs_equal_p (const value_range &) const;
> > >>>>>> /* Like a operator= but update equivalence bitmap efficiently. */
> > >>>>>> void copy_with_equiv_update (const value_range *);
> > >>>>>>
> > >>>>>> /* Types of value ranges. */
> > >>>>>> bool undefined_p () const;
> > >>>>>> bool varying_p () const;
> > >>>>>> bool symbolic_p () const;
> > >>>>>> bool numeric_p () const;
> > >>>>>> void set_undefined (tree = NULL);
> > >>>>>> void set_varying (tree = NULL);
> > >>>>>
> > >>>>> I'd appreciate comments on those predicates, esp. as you
> > >>>>> replace positive tests by negative ones like in
> > >>>>
> > >>>> Done.
> > >>>>
> > >>>>>
> > >>>>> /* If we found any usable VR, set the VR to ssa_name and create a
> > >>>>> PUSH old value in the stack with the old VR. */
> > >>>>> - if (vr.type == VR_RANGE || vr.type == VR_ANTI_RANGE)
> > >>>>> + if (!vr.undefined_p () && !vr.varying_p ())
> > >>>>> {
> > >>>>>
> > >>>>> I'd also spell numeric_p as constant_p or drop it alltogether
> > >>>>> since !symbolic_p should imply it given varying_p and undefined_p
> > >>>>> are just some special-cases of "numeric_p" (full and empty range).
> > >>>>
> > >>>> Done.
> > >>>>
> > >>>>>
> > >>>>> That said, for the time being I'd use non_symbolic_range_or_anti_range_p
> > >>>>> instead of numeric_p () (seeing that you maybe want to hide the fact
> > >>>>> that we have anti-ranges?)
> > >>>>
> > >>>> Errr... No.
> > >>>>
> > >>>>>
> > >>>>> - value_range vr = VR_INITIALIZER;
> > >>>>> + value_range vr (TREE_TYPE (name));
> > >>>>>
> > >>>>> so you basically forgo with the fact that empty ranges are universal?
> > >>>>> I don't like it too much that we have to invent a type here. Why enforce this
> > >>>>> and not allow/force type == NULL_TREE for empty ranges?
> > >>>>>
> > >>>>> One could argue VARYING is also universal to some extent and useful
> > >>>>> only with context, so similar argument applies to your change forcing
> > >>>>> a type for set_value_range_to_varying.
> > >>>>>
> > >>>>> - value_range vr = VR_INITIALIZER;
> > >>>>> + value_range vr;
> > >>>>>
> > >>>>> oh, so you do have a default constructor.
> > >>>>>
> > >>>>>>
> > >>>>>> /* Equivalence bitmap methods. */
> > >>>>>> bitmap equiv () const;
> > >>>>>> void set_equiv (bitmap);
> > >>>>>
> > >>>>> Err, I think we've settled on _not_ wrapping all member accesses
> > >>>>> with get/set methods, didn't we? I personally dislike that very much.
> > >>>>>
> > >>>>>> void equiv_free ();
> > >>>>>> void equiv_copy (const value_range *);
> > >>>>>> void equiv_clear ();
> > >>>>>> void equiv_and (const value_range *);
> > >>>>>> void equiv_ior (const value_range *);
> > >>>>>
> > >>>>> Likewise I find this useless abstraction. It's even questionable
> > >>>>> if _free/_clear/_copy are good APIs here. This should be all
> > >>>>> hidden in intersect/union which I do not find in the API at all...
> > >>>>
> > >>>> I missed that discussion. We did? I dislike exposing the internals.
> > >>>> Abstracting things out makes it easier to change things in the future--
> > >>>> or insert instrumenting code, or whatever.
> > >>>
> > >>> OK, I might misremember and it's eventually just my personal taste
> > >>> against slapping a setFoo/getFoo method in a class as the first
> > >>> thing to do after adding a m_Foo member...
> > >>>
> > >>>> That said, I have removed copy/free/and/or. As you said, it was much
> > >>>> easier to make the details internal to the intersect/union member functions.
> > >>>>
> > >>>> However, I have kept:
> > >>>>
> > >>>> bitmap equiv () const;
> > >>>> void set_equiv (bitmap);
> > >>>> void equiv_clear ();
> > >>>>
> > >>>> I think we can get away with just having a clear, instead of a free, as
> > >>>> it's all in an obstack and there doesn't seem to be any consistent use
> > >>>> of free vs. clear throughout (except one or two, which I've kept).
> > >>>
> > >>> Yeah.
> > >>>
> > >>>> Also, we don't really need to expose set_equiv(), but for its one use in
> > >>>> vr_values::add_equivalence(). One option could be to make vr_values and
> > >>>> value_ranges friends and let add_equivalence touch m_equiv. But that's
> > >>>> a bit heavy handed.
> > >>>>
> > >>>> Or we could add this to the API instead of set_equiv():
> > >>>>
> > >>>> void
> > >>>> value_range::add_equivalence (bitmap_obstack obstack, tree var)
> > >>>> {
> > >>>> }
> > >>>>
> > >>>> I don't know how I feel about passing the obtack, or including
> > >>>> "bitmap.h" from everywhere tree-vrp.h is used (that is, everywhere).
> > >>>
> > >>> Equivalences are evil ;) But I guess passing in the obstack works
> > >>> for me. Maybe as trailing argument, defaulted to NULL in which
> > >>> case we use the default bitmap obstack?
> > >>
> > >> Done.
> > >>
> > >>>
> > >>>> For equiv(), we could remove virtually all of its uses, since 99% of
> > >>>> them are in the form:
> > >>>>
> > >>>> set_value_range (vr, VR_SOMETHING, min, max, vr->equiv ())
> > >>>>
> > >>>> Instead we could We could provide:
> > >>>>
> > >>>> vr->update (VR_SOMETHING, min, max);
> > >>>>
> > >>>> ...which is just like set_value_range, but keeping the equivalences intact.
> > >>>
> > >>> Yep, sounds good.
> > >>
> > >> Done.
> > >>
> > >>>
> > >>>> > hidden in intersect/union which I do not find in the API at all...
> > >>>>
> > >>>> How could you, it was front and center ;-):
> > >>>>
> > >>>> void intersect (const value_range *);
> > >>>> void union_ (const value_range *);
> > >>>
> > >>> Missed that in the first review and then failed to delete that comment ;)
> > >>>
> > >>>>>
> > >>>>>>
> > >>>>>> /* Misc methods. */
> > >>>>>> tree type () const;
> > >>>>>
> > >>>>> type() and vrtype() is confusing - value_type() and range_kind() maybe?
> > >>>>
> > >>>> How about we keep type(), since 99% of all uses of "type" in the
> > >>>> compiler are "tree type", so it's easy to figure out. And instead of
> > >>>> range_kind() we use kind(). It's already obvious it's a range, so
> > >>>> vr->kind() reads fine IMO.
> > >>>
> > >>> Works for me.
> > >>
> > >> Done.
> > >>
> > >>>
> > >>>>>
> > >>>>>> bool null_p () const;
> > >>>>>> bool may_contain_p (tree) const;
> > >>>>>> tree singleton () const;
> > >>>>>
> > >>>>> No documentation? :/ Why null_p but singleton (instead of singleton_p)?
> > >>>>
> > >>>> Documented.
> > >>>>
> > >>>> Singleton returns the singleton if found, otherwise returns NULL.
> > >>>> NULL_P returns true/or false. I thought the preferred way was for _p to
> > >>>> always return booleans.
> > >>>
> > >>> Ah, missed that "detail"...
> > >>>
> > >>>> I don't feel strongly, so I've renamed it to singleton_p() since a
> > >>>> NULL_TREE is as good as false. Another option is:
> > >>>>
> > >>>> bool singleton_p (tree *result = NULL)
> > >>>>
> > >>>> Hmmm...I like this last one. What do you think?
> > >>>
> > >>> Like it as well.
> > >>
> > >> Done.
> > >>
> > >>>
> > >>>>>
> > >>>>>> void set_and_canonicalize (enum value_range_type, tree, tree, tree,
> > >>>>>> bitmap);
> > >>>>>
> > >>>>> Why's that necessary if you enforce sanity?
> > >>>>
> > >>>> Canonicalize also does some optimizations like converting anti-ranges
> > >>>> into ranges if possible. Although I would be OK with putting that
> > >>>> functionality in value_range::set() to be done on creation, I don't know
> > >>>> how I feel about polluting the creation code with fixing swapped min/max:
> > >>>>
> > >>>> /* Wrong order for min and max, to swap them and the VR type we need
> > >>>> to adjust them. */
> > >>>>
> > >>>> It feels wrong to construct a range with swapped end-points, and hope
> > >>>> things turn out ok. ISTM that canonicalize() clearly specifies intent:
> > >>>> I'm giving you a shitty range, fix it.
> > >>>>
> > >>>> Thoughts?
> > >>>
> > >>> OK, let's keep it the way you had it. I never liked this part very much
> > >>> (even though I added it!).
> > >>
> > >> Sounds like you need to have a long talk with yourself ;-).
> > >>
> > >>>
> > >>>>>
> > >>>>>> void dump () const;
> > >>>>>>
> > >>>>>> /* Temporary accessors that should eventually be removed. */
> > >>>>>> enum value_range_type vrtype () const;
> > >>>>>> tree min () const;
> > >>>>>> tree max () const;
> > >>>>>>
> > >>>>>> private:
> > >>>>>> void set (value_range_type, tree type, tree, tree, bitmap);
> > >>>>>> void check ();
> > >>>>>> bool equal_p (const value_range &, bool ignore_equivs) const;
> > >>>>>>
> > >>>>>> enum value_range_type m_vrtype;
> > >>>>>> public:
> > >>>>>> /* These should be private, but GTY is a piece of crap. */
> > >>>>>> tree m_min;
> > >>>>>> tree m_max;
> > >>>>>> tree m_type;
> > >>>>>
> > >>>>> m_type is redundant (see above).
> > >>>>
> > >>>> Removed.
> > >>>>
> > >>>> Tested on x86-64 Linux.
> > >>>>
> > >>>> Aldy
> > >>>>
> > >>>> p.s. Oh yeah, it wouldn't be an Aldy patch without an irrelevant bit
> > >>>> added for good measure:
> > >>>>
> > >>>> +void
> > >>>> +bitmap_head::dump ()
> > >>>> +{
> > >>>> + debug (this);
> > >>>> +}
> > >>>>
> > >>>> I find having ->dump() available for each and every structure in GCC
> > >>>> helpful in debugging. At some point we should standardize on dump(FILE
> > >>>> *) and debug() to dump to stderr. But alas, there are too many dump()'s
> > >>>> that already dump to stderr :-/.
> > >>>
> > >>> FWIW I like
> > >>>
> > >>> void dump (const bitmap_head&);
> > >>>
> > >>> more since it doesn't clutter the APIs and can theoretically be very
> > >>> easily not built into a release compiler. And IIRC we already have
> > >>> global overloads of debug () for exactly the reason you cite. Having
> > >>> both styles is IMHO not good. (and I've stated my preference - feel
> > >>> free to provide statistics for in-tree uses ;))
> > >>
> > >> Ughh, maybe in the future I'll sit down and convert everything to
> > >> something regular.
> > >>
> > >> Tested with all languages on x86-64 Linux.
> > >>
> > >> OK for trunk?
> > >
> > > You seem to remove vr_values::add_equivalence but then...
> > >
> > > diff --git a/gcc/vr-values.h b/gcc/vr-values.h
> > > index 487a800c1ea..496707856c3 100644
> > > --- a/gcc/vr-values.h
> > > +++ b/gcc/vr-values.h
> > > @@ -72,7 +72,7 @@ class vr_values
> > > void cleanup_edges_and_switches (void);
> > >
> > > private:
> > > - void add_equivalence (bitmap *, const_tree);
> > > + bitmap add_equivalence (bitmap, const_tree);
> > > bool vrp_stmt_computes_nonzero (gimple *);
> > > bool op_with_boolean_value_range_p (tree);
> > > bool check_for_binary_op_overflow (enum tree_code, tree, tree, tree, bool *);
> > >
> > > so please remove the method in the class as well.
> > >
> > > OK with that change.
> >
> > I am updating my tree and will commit once a sanity bootstrap succeeds.
> >
> > Thanks so much for your review.
> >
> > Aldy
>
> This caused:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87670
>
> --
> H.J.
More information about the Gcc-patches
mailing list