On Thu, Nov 8, 2018 at 5:03 PM Aldy Hernandez <al...@redhat.com> wrote:
>
>
>
> On 11/8/18 9:39 AM, Richard Biener wrote:
> > On Thu, Nov 8, 2018 at 2:32 PM Aldy Hernandez <al...@redhat.com> wrote:
> >>
> >> [Richard, you're right. An overloaded debug() is better than
> >> this->dump(). Anyone who thinks different is wrong. End of discussion.]
> >>
> >> There's no reason to have multiple ways of dumping a value range. And
> >> I'm not even talking about ipa-prop.* which decided that they should
> >> implement their own version of value_ranges basically to annoy me.
> >> Attached is a patch to remedy this.
> >>
> >> I have made three small user-visible changes to the dumping code--
> >> cause, well... you know me... I can't leave things alone.
> >>
> >> 1. It *REALLY* irks me that bools are dumped with -INF/+INF. Everyone
> >> knows that bools should be 0/1. It's in the Bible. Look it up.
> >>
> >> 2. Analogous to #1, what's up with signed 1-bit fields? Who understands
> >> [0, +INF] as [0, 0]? No one; that's right. (It's still beyond me the
> >> necessity of signed bit fields in the standard period, but I'll pick my
> >> battles.)
> >>
> >> 3. I find it quite convenient to display the tree type along with the
> >> range as:
> >>
> >> [1, 1] int EQUIVALENCES { me, myself, I }
> >
> > the type inbetween the range and equivalences? seriously? If so then
> > _please_ dump the type before the range:
> >
> > int [1, 1] EQUIV { ... }
>
> Done in attached patch.
>
> >
> >> Finally.. As mentioned, I've implement an overloaded debug() because
> >> it's *so* nice to use gdb and an alias of:
> >>
> >> define dd
> >> call debug($arg0)
> >> end
> >>
> >> ...and have stuff just work.
> >>
> >> I do realize that we still have dump() lying around. At some point, we
> >> should start dropping external access to the dump methods for objects
> >> that are never dumped by the compiler (but by the user at debug time).
> >
> > +DEBUG_FUNCTION void
> > +debug (const value_range *vr)
> > +{
> > + dump_value_range (stderr, vr);
> > +}
> > +
> > +DEBUG_FUNCTION void
> > +debug (const value_range vr)
> >
> > ^^^ missing a & (const reference?)
> >
> > +{
> > + dump_value_range (stderr, &vr);
> > +}
>
> Fixed.
>
> >
> > also
> >
> > @@ -2122,21 +2122,11 @@ dump_ssaname_info (pretty_printer *buffer,
> > tree node, int spc)
> > if (!POINTER_TYPE_P (TREE_TYPE (node))
> > && SSA_NAME_RANGE_INFO (node))
> > {
> > - wide_int min, max, nonzero_bits;
> > - value_range_kind range_type = get_range_info (node, &min, &max);
> > + value_range vr;
> >
> > - if (range_type == VR_VARYING)
> > - pp_printf (buffer, "# RANGE VR_VARYING");
> > - else if (range_type == VR_RANGE || range_type == VR_ANTI_RANGE)
> > - {
> > - pp_printf (buffer, "# RANGE ");
> > - pp_printf (buffer, "%s[", range_type == VR_RANGE ? "" : "~");
> > - pp_wide_int (buffer, min, TYPE_SIGN (TREE_TYPE (node)));
> > - pp_printf (buffer, ", ");
> > - pp_wide_int (buffer, max, TYPE_SIGN (TREE_TYPE (node)));
> > - pp_printf (buffer, "]");
> > - }
> > - nonzero_bits = get_nonzero_bits (node);
> > + get_range_info (node, vr);
> >
> > this is again allocating INTEGER_CSTs for no good reason. dumping really
> > shuld avoid possibly messing with the GC state.
>
> Hmmmm, I'd really hate to have two distinct places that dump value
> ranges. Is this really a problem? Cause the times I've run into GC
> problems I'd had to resort to printf() anyhow...
Well, if you can avoid GC avoid it. If you are in the midst of debugging
you certainly do not want your inferior calls mess with the GC state.
> And while we're on the subject of multiple implementations, I'm thinking
> of rewriting ipa-prop to actually use value_range's, not this:
>
> struct GTY(()) ipa_vr
> {
> /* The data fields below are valid only if known is true. */
> bool known;
> enum value_range_kind type;
> wide_int min;
> wide_int max;
> };
>
> so perhaps:
>
> struct { bool known; value_range vr; }
Size is of great matter here so m_equiv stands in the way here. Also that
would exchange wide-ints for trees. Were trees not bad? Andrew?!?!
> Remember that trees are cached, whereas wide_int's are not, and folks
> (not me :)) like to complain that wide_int's take a lot of space.
There's trailing-wide-ints for this issue.
> Would you be viscerally opposed to implementing ipa_vr with
> value_range's? I really hate having to deal with yet another
> value_range variant.
Well, fact is that the SSA annotations have integer constant ranges
only, and those are transfered by IPA. Exchanging that for the
fat internal representation of VRP is stupid.
You started separating out that wide-int-range stuff and _this_ is
what the SSA annotations / IPA should use.
The value_range class from VRP should become more internal again.
I guess I'm not seeing the end of the road you are currently walking?
Richard.
> Aldy
