From: Aldy Hernandez <aldyh@redhat.com>
To: Richard Biener <richard.guenther@gmail.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>,
Andrew MacLeod <amacleod@redhat.com>
Subject: Re: [patch] new API for value_range
Date: Wed, 17 Oct 2018 15:46:00 -0000 [thread overview]
Message-ID: <16b7823a-bd57-de96-63a9-de92c26e8f9f@redhat.com> (raw)
In-Reply-To: <CAFiYyc1i8eAEZFbTtwSte6h-9DXdU=YgCOun+hdWN=uZhtqdRg@mail.gmail.com>
On 10/17/18 6:50 AM, Richard Biener wrote:
> 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.
I am updating my tree and will commit once a sanity bootstrap succeeds.
Thanks so much for your review.
Aldy
next prev parent reply other threads:[~2018-10-17 14:39 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-09 17:04 Aldy Hernandez
2018-10-10 11:17 ` Richard Biener
2018-10-11 9:47 ` Aldy Hernandez
2018-10-11 10:12 ` Richard Biener
2018-10-11 18:33 ` Aldy Hernandez
2018-10-17 10:17 ` PING: Fwd: " Aldy Hernandez
2018-10-17 12:06 ` Richard Biener
2018-10-17 15:46 ` Aldy Hernandez [this message]
2018-10-21 8:51 ` H.J. Lu
2018-10-21 17:48 ` Aldy Hernandez
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=16b7823a-bd57-de96-63a9-de92c26e8f9f@redhat.com \
--to=aldyh@redhat.com \
--cc=amacleod@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=richard.guenther@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).