From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 54933 invoked by alias); 17 Oct 2018 10:50:52 -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 54913 invoked by uid 89); 17 Oct 2018 10:50:51 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.2 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=it!, evil, Hey, hmmm X-HELO: mail-lf1-f43.google.com Received: from mail-lf1-f43.google.com (HELO mail-lf1-f43.google.com) (209.85.167.43) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 17 Oct 2018 10:50:48 +0000 Received: by mail-lf1-f43.google.com with SMTP id v22-v6so7611228lfe.12 for ; Wed, 17 Oct 2018 03:50:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=RhMZ7/D5dYo9Q667mry2QgdCgI0+wiSggKID9E0JKK8=; b=CP1U3vzYtwsrGv3QeUbtLAYYgQMFaxPRcwZXEnZkaH9AdBbJNsLKa8fS2LbxYeYaZv asAucnjI0IZvRKNqZW1rbscjczJmO+kU6xTjU/yVhPQ+xLnp+53qnEkAxVeGN1zkVBJc OM7u5myr5ENfir8LTdvXGWRBmb5leKOlJQzfLEcX3uO2JmmFuTjFe8sxezYLno6uVqtg NbBCeUQqLAycv7cgygZ+K0pIkmc2OJV+fKSku6rRMmBYExzL/pEFbPenKIUs1Qi2FgVl h5HZIHAX4+/A7b2LiGhwu4txDyKSjh8AWXfmB2WoP0/FkUuKFfrZOcM3NpK6SlDGD7rm yrag== MIME-Version: 1.0 References: <83e85260-a847-c848-27e7-148a8edc72bb@redhat.com> <91d2b68a-7382-fd6e-e936-c3f79d71593b@redhat.com> In-Reply-To: <91d2b68a-7382-fd6e-e936-c3f79d71593b@redhat.com> From: Richard Biener Date: Wed, 17 Oct 2018 12:06:00 -0000 Message-ID: Subject: Re: [patch] new API for value_range To: Aldy Hernandez Cc: GCC Patches , Andrew MacLeod Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2018-10/txt/msg01031.txt.bz2 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. Richard. > Aldy