From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 113715 invoked by alias); 21 Oct 2018 14:39:18 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 113699 invoked by uid 89); 21 Oct 2018 14:39:17 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy= X-HELO: mail-ot1-f50.google.com Received: from mail-ot1-f50.google.com (HELO mail-ot1-f50.google.com) (209.85.210.50) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 21 Oct 2018 14:39:14 +0000 Received: by mail-ot1-f50.google.com with SMTP id l58so37584684otd.6 for ; Sun, 21 Oct 2018 07:39:14 -0700 (PDT) MIME-Version: 1.0 References: <83e85260-a847-c848-27e7-148a8edc72bb@redhat.com> <91d2b68a-7382-fd6e-e936-c3f79d71593b@redhat.com> <16b7823a-bd57-de96-63a9-de92c26e8f9f@redhat.com> In-Reply-To: From: Aldy Hernandez Date: Sun, 21 Oct 2018 17:48:00 -0000 Message-ID: Subject: Re: [patch] new API for value_range To: "H.J. Lu" Cc: Richard Biener , gcc-patches , "MacLeod, Andrew" Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2018-10/txt/msg01277.txt.bz2 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 wrote: > > On Wed, Oct 17, 2018 at 7:39 AM Aldy Hernandez wrote: > > > > > > > > On 10/17/18 6:50 AM, Richard Biener wrote: > > > On Thu, Oct 11, 2018 at 8:25 PM Aldy Hernandez wrote: > > >> > > >> > > >> > > >> On 10/11/18 5:47 AM, Richard Biener wrote: > > >>> On Thu, Oct 11, 2018 at 10:19 AM Aldy Hernandez 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 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.