From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26464 invoked by alias); 17 Oct 2018 14:39:21 -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 26446 invoked by uid 89); 17 Oct 2018 14:39:19 -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-wr1-f42.google.com Received: from mail-wr1-f42.google.com (HELO mail-wr1-f42.google.com) (209.85.221.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 17 Oct 2018 14:39:16 +0000 Received: by mail-wr1-f42.google.com with SMTP id n1-v6so29986296wrt.10 for ; Wed, 17 Oct 2018 07:39:15 -0700 (PDT) Return-Path: Received: from abulafia.quesejoda.com (28.red-83-50-201.dynamicip.rima-tde.net. [83.50.201.28]) by smtp.gmail.com with ESMTPSA id r131-v6sm3274643wmb.32.2018.10.17.07.39.12 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 17 Oct 2018 07:39:13 -0700 (PDT) Subject: Re: [patch] new API for value_range To: Richard Biener Cc: GCC Patches , Andrew MacLeod References: <83e85260-a847-c848-27e7-148a8edc72bb@redhat.com> <91d2b68a-7382-fd6e-e936-c3f79d71593b@redhat.com> From: Aldy Hernandez Message-ID: <16b7823a-bd57-de96-63a9-de92c26e8f9f@redhat.com> Date: Wed, 17 Oct 2018 15:46:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-10/txt/msg01046.txt.bz2 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