On Wed, Oct 17, 2018 at 7:39 AM Aldy Hernandez <al...@redhat.com> wrote: > > > > On 10/17/18 6:50 AM, Richard Biener wrote: > > On Thu, Oct 11, 2018 at 8:25 PM Aldy Hernandez <al...@redhat.com> wrote: > >> > >> > >> > >> On 10/11/18 5:47 AM, Richard Biener wrote: > >>> On Thu, Oct 11, 2018 at 10:19 AM Aldy Hernandez <al...@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 <al...@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.
