[patch] new API for value_range
Richard Biener
richard.guenther@gmail.com
Wed Oct 17 12:06:00 GMT 2018
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.
Richard.
> Aldy
More information about the Gcc-patches
mailing list