cleanups and unification of value_range dumping code

[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 }

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).

OK for trunk?

On Thu, Nov 8, 2018 at 2:32 PM Aldy Hernandez <aldyh@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 { ... }

> 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);
+}

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.

wide-int-range again?

Richard.

> OK for trunk?

On 11/8/18 9:39 AM, Richard Biener wrote:
> On Thu, Nov 8, 2018 at 2:32 PM Aldy Hernandez <aldyh@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...

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; }

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.

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.

Aldy
* gimple-pretty-print.c (dump_ssaname_info): Use value_range
            dumper to dump range information.
            * tree-vrp.c (value_range::dump_range): New.
            (value_range::dump): Add variant for dumping to pretty_printer.
            Adjust FILE * version accordingly.
            (debug_value_range): Rename to debug.
    
            Remove unused prototypes: dump_value_range, debug_value_range,
            dump_all_value_ranges, dump_vr_equiv, debug_vr_equiv.
            * tree-vrp.h (value_range::dump): Add new variant that takes
            pretty_printer.
            (value_range::dump_range): New.

diff --git a/gcc/gimple-pretty-print.c b/gcc/gimple-pretty-print.c
index 7dfec9120ab..73af30ba92f 100644
--- a/gcc/gimple-pretty-print.c
+++ b/gcc/gimple-pretty-print.c
@@ -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);
+      vr.dump (buffer);
+      wide_int nonzero_bits = get_nonzero_bits (node);
       if (nonzero_bits != -1)
 	{
 	  pp_string (buffer, " NONZERO ");
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp92.c b/gcc/testsuite/gcc.dg/tree-ssa/vrp92.c
index 5a2dbf0108a..66d74e9b5e9 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/vrp92.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp92.c
@@ -18,5 +18,5 @@ int foo (int i, int j)
   return j;
 }
 
-/* { dg-final { scan-tree-dump "res_.: \\\[1, 1\\\]" "vrp1" } } */
+/* { dg-final { scan-tree-dump "res_.: int \\\[1, 1\\\]" "vrp1" } } */
 /* { dg-final { scan-tree-dump-not "Threaded" "vrp1" } } */
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 3ccf2e491d6..b13f7205570 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -310,57 +310,70 @@ value_range::type () const
   return TREE_TYPE (min ());
 }
 
-/* Dump value range to FILE.  */
+void
+value_range::dump_range (pretty_printer *buffer) const
+{
+  tree ttype = type ();
+  dump_generic_node (buffer, ttype, 0, TDF_SLIM, false);
+  pp_character (buffer, ' ');
+  pp_printf (buffer, "%s[", (m_kind == VR_ANTI_RANGE) ? "~" : "");
+  if (INTEGRAL_TYPE_P (ttype)
+      && !TYPE_UNSIGNED (ttype)
+      && vrp_val_is_min (min ())
+      && TYPE_PRECISION (ttype) != 1)
+    pp_printf (buffer, "-INF");
+  else
+    dump_generic_node (buffer, min (), 0, TDF_SLIM, false);
+
+  pp_printf (buffer, ", ");
+
+  if (INTEGRAL_TYPE_P (ttype)
+      && vrp_val_is_max (max ())
+      && TYPE_PRECISION (ttype) != 1)
+    pp_printf (buffer, "+INF");
+  else
+    dump_generic_node (buffer, max (), 0, TDF_SLIM, false);
+  pp_string (buffer, "] ");
+}
 
 void
-value_range::dump (FILE *file) const
+value_range::dump (pretty_printer *buffer) const
 {
   if (undefined_p ())
-    fprintf (file, "UNDEFINED");
+    pp_string (buffer, "UNDEFINED");
   else if (m_kind == VR_RANGE || m_kind == VR_ANTI_RANGE)
     {
-      tree type = TREE_TYPE (min ());
-
-      fprintf (file, "%s[", (m_kind == VR_ANTI_RANGE) ? "~" : "");
-
-      if (INTEGRAL_TYPE_P (type)
-	  && !TYPE_UNSIGNED (type)
-	  && vrp_val_is_min (min ()))
-	fprintf (file, "-INF");
-      else
-	print_generic_expr (file, min ());
-
-      fprintf (file, ", ");
-
-      if (INTEGRAL_TYPE_P (type)
-	  && vrp_val_is_max (max ()))
-	fprintf (file, "+INF");
-      else
-	print_generic_expr (file, max ());
-
-      fprintf (file, "]");
-
+      dump_range (buffer);
       if (m_equiv)
 	{
 	  bitmap_iterator bi;
 	  unsigned i, c = 0;
 
-	  fprintf (file, "  EQUIVALENCES: { ");
+	  pp_printf (buffer, " EQUIVALENCES: { ");
 
 	  EXECUTE_IF_SET_IN_BITMAP (m_equiv, 0, i, bi)
 	    {
-	      print_generic_expr (file, ssa_name (i));
-	      fprintf (file, " ");
+	      dump_generic_node (buffer, ssa_name (i), 0, TDF_SLIM, false);
+	      pp_string (buffer, " ");
 	      c++;
 	    }
 
-	  fprintf (file, "} (%u elements)", c);
+	  pp_printf (buffer, "} (%u elements)", c);
 	}
     }
   else if (varying_p ())
-    fprintf (file, "VARYING");
+    pp_string (buffer, "VARYING");
+  else
+    pp_string (buffer, "INVALID RANGE");
+}
+
+void
+dump_value_range (FILE *file, const value_range *vr)
+{
+  if (!vr)
+    fprintf (file, "[]");
   else
-    fprintf (file, "INVALID RANGE");
+    vr->dump (file);
 }
 
 void
@@ -370,6 +383,28 @@ value_range::dump () const
   fprintf (stderr, "\n");
 }
 
+void
+value_range::dump (FILE *file) const
+{
+ pretty_printer buffer;
+ pp_needs_newline (&buffer) = true;
+ buffer.buffer->stream = file;
+ dump (&buffer);
+ pp_flush (&buffer);
+}
+
+DEBUG_FUNCTION void
+debug (const value_range *vr)
+{
+  dump_value_range (stderr, vr);
+}
+
+DEBUG_FUNCTION void
+debug (const value_range &vr)
+{
+  dump_value_range (stderr, &vr);
+}
+
 /* Return true if the SSA name NAME is live on the edge E.  */
 
 static bool
@@ -2102,32 +2137,6 @@ extract_range_from_unary_expr (value_range *vr,
   return;
 }
 
-/* Debugging dumps.  */
-
-void dump_value_range (FILE *, const value_range *);
-void debug_value_range (const value_range *);
-void dump_all_value_ranges (FILE *);
-void dump_vr_equiv (FILE *, bitmap);
-void debug_vr_equiv (bitmap);
-
-void
-dump_value_range (FILE *file, const value_range *vr)
-{
-  if (!vr)
-    fprintf (file, "[]");
-  else
-    vr->dump (file);
-}
-
-/* Dump value range VR to stderr.  */
-
-DEBUG_FUNCTION void
-debug_value_range (const value_range *vr)
-{
-  vr->dump ();
-}
-
-
 /* Given a COND_EXPR COND of the form 'V OP W', and an SSA name V,
    create a new SSA name N and return the assertion assignment
    'N = ASSERT_EXPR <V, V OP W>'.  */
diff --git a/gcc/tree-vrp.h b/gcc/tree-vrp.h
index c251329a195..934097523dd 100644
--- a/gcc/tree-vrp.h
+++ b/gcc/tree-vrp.h
@@ -71,6 +71,7 @@ class GTY((for_user)) value_range
   void set_and_canonicalize (enum value_range_kind, tree, tree, bitmap);
   void dump (FILE *) const;
   void dump () const;
+  void dump (pretty_printer *buffer) const;
 
   enum value_range_kind kind () const;
   tree min () const;
@@ -82,6 +83,7 @@ class GTY((for_user)) value_range
   bool equal_p (const value_range &, bool ignore_equivs) const;
   void intersect_helper (value_range *, const value_range *);
   void union_helper (value_range *, const value_range *);
+  void dump_range (pretty_printer *) const;
 
   enum value_range_kind m_kind;
  public:

On Thu, Nov 8, 2018 at 5:03 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
>
>
> On 11/8/18 9:39 AM, Richard Biener wrote:
> > On Thu, Nov 8, 2018 at 2:32 PM Aldy Hernandez <aldyh@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

On 11/9/18 6:37 AM, Richard Biener wrote:
> On Thu, Nov 8, 2018 at 5:03 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>
>>
>>
>> On 11/8/18 9:39 AM, Richard Biener wrote:
>>> On Thu, Nov 8, 2018 at 2:32 PM Aldy Hernandez <aldyh@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.

I personally haven't had to deal with this in 20 years, and I believe 
the convenience of having one dumping routine makes up for the 
theoretical possibility that we'd run into this in the future.

> 
>> 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.

Had we been using trailing wide ints, it would've been a great argument, 
but alas we aren't.  As it stands, we'd use 44% less memory by using 
value_range's-- even with the m_equiv waste:

struct GTY(()) ipa_with_value_range
{
   bool known;
   value_range vr;
};

sizeof(ipa_vr)			=> 72 bytes
sizeof(ipa_with_value_range)	=> 40 bytes

Also, won't your upcoming value_range_without_equivs work further reduce 
these 40 bytes?

Aldy

I have rebased my value_range dumping patch after your value_range_base 
changes.

I know you are not a fan of the gimple-pretty-print.c chunk, but I still 
think having one set of dumping code is preferable to catering to 
possible GC corruption while debugging.  If you're strongly opposed (as, 
I'm putting my foot down), I can remove it as well as the relevant 
pretty_printer stuff.

The patch looks bigger than it is because I moved all the dump routines 
into one place.

OK?

p.s. After your changes, perhaps get_range_info(const_tree, value_range 
&) should take a value_range_base instead?
gcc/

	* gimple-pretty-print.c (dump_ssaname_info): Use value_range
	dumping infrastructure.
	* ipa-cp.c (ipcp_vr_lattice::print): Call overloaded
	dump_value_range.
	* tree-vrp.c (value_range_base::dump): Rewrite to use
	pretty_printer.  Dump type.  Do not display -INF/+INF if precision
	is 1.
	Move all dumping routines into one spot.
	* tree-vrp.h (value_range_base): Add pretty_printer variant.
	(value_range): Same.
	(dump_value_range_base): Rename to overloaded dump_value_range.

gcc/testsuite/

	* gcc.dg/tree-ssa/pr64130.c: Adjust for new value_range pretty
	printer.
	* gcc.dg/tree-ssa/vrp92.c: Same.

diff --git a/gcc/gimple-pretty-print.c b/gcc/gimple-pretty-print.c
index 276e5798bac..e69683f174e 100644
--- a/gcc/gimple-pretty-print.c
+++ b/gcc/gimple-pretty-print.c
@@ -2151,21 +2151,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);
+      vr.dump (buffer);
+      wide_int nonzero_bits = get_nonzero_bits (node);
       if (nonzero_bits != -1)
 	{
 	  pp_string (buffer, " NONZERO ");
diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 4f147eb37cc..882c8975ff4 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -522,7 +522,7 @@ ipcp_bits_lattice::print (FILE *f)
 void
 ipcp_vr_lattice::print (FILE * f)
 {
-  dump_value_range_base (f, &m_vr);
+  dump_value_range (f, &m_vr);
 }
 
 /* Print all ipcp_lattices of all functions to F.  */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr64130.c b/gcc/testsuite/gcc.dg/tree-ssa/pr64130.c
index e068765e2fc..28ffbb76da8 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr64130.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr64130.c
@@ -15,6 +15,6 @@ int funsigned2 (uint32_t a)
   return (-1 * 0x1ffffffffL) / a == 0;
 }
 
-/* { dg-final { scan-tree-dump ": \\\[2, 8589934591\\\]" "evrp" } } */
-/* { dg-final { scan-tree-dump ": \\\[-8589934591, -2\\\]" "evrp" } } */
+/* { dg-final { scan-tree-dump "int \\\[2, 8589934591\\\]" "evrp" } } */
+/* { dg-final { scan-tree-dump "int \\\[-8589934591, -2\\\]" "evrp" } } */
 
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp92.c b/gcc/testsuite/gcc.dg/tree-ssa/vrp92.c
index 5a2dbf0108a..66d74e9b5e9 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/vrp92.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp92.c
@@ -18,5 +18,5 @@ int foo (int i, int j)
   return j;
 }
 
-/* { dg-final { scan-tree-dump "res_.: \\\[1, 1\\\]" "vrp1" } } */
+/* { dg-final { scan-tree-dump "res_.: int \\\[1, 1\\\]" "vrp1" } } */
 /* { dg-final { scan-tree-dump-not "Threaded" "vrp1" } } */
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 3ef676bb71b..0081821985c 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -358,70 +358,130 @@ value_range_base::type () const
   return TREE_TYPE (min ());
 }
 
-/* Dump value range to FILE.  */
+/* Value range dumping functions.  */
 
 void
-value_range_base::dump (FILE *file) const
+value_range_base::dump (pretty_printer *buffer) const
 {
   if (undefined_p ())
-    fprintf (file, "UNDEFINED");
+    pp_string (buffer, "UNDEFINED");
   else if (m_kind == VR_RANGE || m_kind == VR_ANTI_RANGE)
     {
-      tree type = TREE_TYPE (min ());
+      tree ttype = type ();
+
+      dump_generic_node (buffer, ttype, 0, TDF_SLIM, false);
+      pp_character (buffer, ' ');
 
-      fprintf (file, "%s[", (m_kind == VR_ANTI_RANGE) ? "~" : "");
+      pp_printf (buffer, "%s[", (m_kind == VR_ANTI_RANGE) ? "~" : "");
 
-      if (INTEGRAL_TYPE_P (type)
-	  && !TYPE_UNSIGNED (type)
-	  && vrp_val_is_min (min ()))
-	fprintf (file, "-INF");
+      if (INTEGRAL_TYPE_P (ttype)
+	  && !TYPE_UNSIGNED (ttype)
+	  && vrp_val_is_min (min ())
+	  && TYPE_PRECISION (ttype) != 1)
+	pp_printf (buffer, "-INF");
       else
-	print_generic_expr (file, min ());
+	dump_generic_node (buffer, min (), 0, TDF_SLIM, false);
 
-      fprintf (file, ", ");
+      pp_printf (buffer, ", ");
 
-      if (INTEGRAL_TYPE_P (type)
-	  && vrp_val_is_max (max ()))
-	fprintf (file, "+INF");
+      if (INTEGRAL_TYPE_P (ttype)
+	  && vrp_val_is_max (max ())
+	  && TYPE_PRECISION (ttype) != 1)
+	pp_printf (buffer, "+INF");
       else
-	print_generic_expr (file, max ());
+	dump_generic_node (buffer, max (), 0, TDF_SLIM, false);
 
-      fprintf (file, "]");
+      pp_string (buffer, "]");
     }
   else if (varying_p ())
-    fprintf (file, "VARYING");
+    pp_string (buffer, "VARYING");
   else
-    fprintf (file, "INVALID RANGE");
+    pp_string (buffer, "INVALID RANGE");
 }
 
 void
-value_range::dump (FILE *file) const
+dump_value_range (FILE *file, const value_range_base *vr)
+{
+  if (!vr)
+    fprintf (file, "[]");
+  else
+    vr->dump (file);
+}
+
+void
+dump_value_range (FILE *file, const value_range *vr)
+{
+  if (!vr)
+    fprintf (file, "[]");
+  else
+    vr->dump (file);
+}
+
+void
+value_range::dump (pretty_printer *buffer) const
 {
-  value_range_base::dump (file);
+  value_range_base::dump (buffer);
   if ((m_kind == VR_RANGE || m_kind == VR_ANTI_RANGE)
       && m_equiv)
     {
       bitmap_iterator bi;
       unsigned i, c = 0;
 
-      fprintf (file, "  EQUIVALENCES: { ");
+      pp_printf (buffer, "  EQUIVALENCES: { ");
 
       EXECUTE_IF_SET_IN_BITMAP (m_equiv, 0, i, bi)
 	{
-	  print_generic_expr (file, ssa_name (i));
-	  fprintf (file, " ");
+	  dump_generic_node (buffer, ssa_name (i), 0, TDF_SLIM, false);
+	  pp_character (buffer, ' ');
 	  c++;
 	}
 
-      fprintf (file, "} (%u elements)", c);
+      pp_printf (buffer, "} (%u elements)", c);
     }
 }
 
 void
-value_range::dump () const
+value_range_base::dump (FILE *file) const
+{
+ pretty_printer buffer;
+ pp_needs_newline (&buffer) = true;
+ buffer.buffer->stream = file;
+ dump (&buffer);
+ pp_flush (&buffer);
+}
+
+void
+value_range::dump (FILE *file) const
+{
+ pretty_printer buffer;
+ pp_needs_newline (&buffer) = true;
+ buffer.buffer->stream = file;
+ dump (&buffer);
+ pp_flush (&buffer);
+}
+
+DEBUG_FUNCTION void
+debug (const value_range_base *vr)
+{
+  dump_value_range (stderr, vr);
+}
+
+DEBUG_FUNCTION void
+debug (const value_range_base &vr)
 {
-  dump_value_range (stderr, this);
-  fprintf (stderr, "\n");
+  dump_value_range (stderr, &vr);
+}
+
+DEBUG_FUNCTION void
+debug (const value_range *vr)
+{
+  dump_value_range (stderr, vr);
+}
+
+DEBUG_FUNCTION void
+debug (const value_range &vr)
+{
+  dump_value_range (stderr, &vr);
 }
 
 /* Return true if the SSA name NAME is live on the edge E.  */
@@ -2167,41 +2227,6 @@ extract_range_from_unary_expr (value_range *vr,
   return;
 }
 
-/* Debugging dumps.  */
-
-void dump_value_range (FILE *, const value_range *);
-void debug_value_range (const value_range *);
-void dump_all_value_ranges (FILE *);
-void dump_vr_equiv (FILE *, bitmap);
-void debug_vr_equiv (bitmap);
-
-void
-dump_value_range (FILE *file, const value_range *vr)
-{
-  if (!vr)
-    fprintf (file, "[]");
-  else
-    vr->dump (file);
-}
-
-void
-dump_value_range_base (FILE *file, const value_range_base *vr)
-{
-  if (!vr)
-    fprintf (file, "[]");
-  else
-    vr->dump (file);
-}
-
-/* Dump value range VR to stderr.  */
-
-DEBUG_FUNCTION void
-debug_value_range (const value_range *vr)
-{
-  vr->dump ();
-}
-
-
 /* Given a COND_EXPR COND of the form 'V OP W', and an SSA name V,
    create a new SSA name N and return the assertion assignment
    'N = ASSERT_EXPR <V, V OP W>'.  */
diff --git a/gcc/tree-vrp.h b/gcc/tree-vrp.h
index 1e141c017e8..d93557ac83c 100644
--- a/gcc/tree-vrp.h
+++ b/gcc/tree-vrp.h
@@ -63,8 +63,8 @@ public:
   tree type () const;
   bool may_contain_p (tree) const;
   void set_and_canonicalize (enum value_range_kind, tree, tree);
-
   void dump (FILE *) const;
+  void dump (pretty_printer *buffer) const;
 
 protected:
   void set (value_range_kind, tree, tree);
@@ -116,7 +116,7 @@ class GTY((user)) value_range : public value_range_base
   void deep_copy (const value_range *);
   void set_and_canonicalize (enum value_range_kind, tree, tree, bitmap);
   void dump (FILE *) const;
-  void dump () const;
+  void dump (pretty_printer *) const;
 
  private:
   void set (value_range_kind, tree, tree, bitmap);
@@ -202,7 +202,7 @@ value_range::zero_p () const
 }
 
 extern void dump_value_range (FILE *, const value_range *);
-extern void dump_value_range_base (FILE *, const value_range_base *);
+extern void dump_value_range (FILE *, const value_range_base *);
 extern void extract_range_from_unary_expr (value_range *vr,
 					   enum tree_code code,
 					   tree type,

On Mon, Nov 12, 2018 at 10:50 AM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> I have rebased my value_range dumping patch after your value_range_base
> changes.
>
> I know you are not a fan of the gimple-pretty-print.c chunk, but I still
> think having one set of dumping code is preferable to catering to
> possible GC corruption while debugging.  If you're strongly opposed (as,
> I'm putting my foot down), I can remove it as well as the relevant
> pretty_printer stuff.

I'd say we do not want to change the gimple-pretty-print.c stuff also because
we'll miss the leading #.  I'd rather see a simple wide-int-range class
wrapping the interesting bits up.  I guess I'll come up with one then ;)

> The patch looks bigger than it is because I moved all the dump routines
> into one place.
>
> OK?
>
> p.s. After your changes, perhaps get_range_info(const_tree, value_range
> &) should take a value_range_base instead?

Yes, I missed that and am now testing this change.

Btw, the patch needs updating again (sorry).  If you leave out the
gimple-pretty-print.c stuff there's no requirement to use the pretty-printer
API, right?

Richard.

On 11/13/18 3:12 AM, Richard Biener wrote:
> On Mon, Nov 12, 2018 at 10:50 AM Aldy Hernandez <aldyh@redhat.com> wrote:
>>
>> I have rebased my value_range dumping patch after your value_range_base
>> changes.
>>
>> I know you are not a fan of the gimple-pretty-print.c chunk, but I still
>> think having one set of dumping code is preferable to catering to
>> possible GC corruption while debugging.  If you're strongly opposed (as,
>> I'm putting my foot down), I can remove it as well as the relevant
>> pretty_printer stuff.
> 
> I'd say we do not want to change the gimple-pretty-print.c stuff also because
> we'll miss the leading #.  I'd rather see a simple wide-int-range class
> wrapping the interesting bits up.  I guess I'll come up with one then ;)

Ok.  Removed.

> 
>> The patch looks bigger than it is because I moved all the dump routines
>> into one place.
>>
>> OK?
>>
>> p.s. After your changes, perhaps get_range_info(const_tree, value_range
>> &) should take a value_range_base instead?
> 
> Yes, I missed that and am now testing this change.

Thanks.

> 
> Btw, the patch needs updating again (sorry).  If you leave out the
> gimple-pretty-print.c stuff there's no requirement to use the pretty-printer
> API, right?

No need to apologize for contributing code :).  Thanks.  And yes, 
there's no need for the pretty-printer bits.

I've also removed the value_range*::dump() versions with no arguments, 
as now we have an overloaded debug() for use from the debugger.

Testing attached patch.

Aldy
gcc/

	* tree-vrp.c (value_range_base::dump): Dump type.
	Do not use INF nomenclature for 1-bit types.
	(dump_value_range): Group all variants to common dumping code.
	(debug): New overloaded functions for value_ranges.
	(value_range_base::dump): Remove no argument version.
	(value_range::dump): Same.

gcc/testsuite/

	* gcc.dg/tree-ssa/pr64130.c: Adjust for new value_range pretty
	printer.
	* gcc.dg/tree-ssa/vrp92.c: Same.

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr64130.c b/gcc/testsuite/gcc.dg/tree-ssa/pr64130.c
index e068765e2fc..28ffbb76da8 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr64130.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr64130.c
@@ -15,6 +15,6 @@ int funsigned2 (uint32_t a)
   return (-1 * 0x1ffffffffL) / a == 0;
 }
 
-/* { dg-final { scan-tree-dump ": \\\[2, 8589934591\\\]" "evrp" } } */
-/* { dg-final { scan-tree-dump ": \\\[-8589934591, -2\\\]" "evrp" } } */
+/* { dg-final { scan-tree-dump "int \\\[2, 8589934591\\\]" "evrp" } } */
+/* { dg-final { scan-tree-dump "int \\\[-8589934591, -2\\\]" "evrp" } } */
 
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp92.c b/gcc/testsuite/gcc.dg/tree-ssa/vrp92.c
index 5a2dbf0108a..66d74e9b5e9 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/vrp92.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp92.c
@@ -18,5 +18,5 @@ int foo (int i, int j)
   return j;
 }
 
-/* { dg-final { scan-tree-dump "res_.: \\\[1, 1\\\]" "vrp1" } } */
+/* { dg-final { scan-tree-dump "res_.: int \\\[1, 1\\\]" "vrp1" } } */
 /* { dg-final { scan-tree-dump-not "Threaded" "vrp1" } } */
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 27bc1769f11..f498386e8eb 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -365,8 +365,6 @@ value_range_base::type () const
   return TREE_TYPE (min ());
 }
 
-/* Dump value range to FILE.  */
-
 void
 value_range_base::dump (FILE *file) const
 {
@@ -374,21 +372,26 @@ value_range_base::dump (FILE *file) const
     fprintf (file, "UNDEFINED");
   else if (m_kind == VR_RANGE || m_kind == VR_ANTI_RANGE)
     {
-      tree type = TREE_TYPE (min ());
+      tree ttype = type ();
+
+      print_generic_expr (file, ttype);
+      fprintf (file, " ");
 
       fprintf (file, "%s[", (m_kind == VR_ANTI_RANGE) ? "~" : "");
 
-      if (INTEGRAL_TYPE_P (type)
-	  && !TYPE_UNSIGNED (type)
-	  && vrp_val_is_min (min ()))
+      if (INTEGRAL_TYPE_P (ttype)
+	  && !TYPE_UNSIGNED (ttype)
+	  && vrp_val_is_min (min ())
+	  && TYPE_PRECISION (ttype) != 1)
 	fprintf (file, "-INF");
       else
 	print_generic_expr (file, min ());
 
       fprintf (file, ", ");
 
-      if (INTEGRAL_TYPE_P (type)
-	  && vrp_val_is_max (max ()))
+      if (INTEGRAL_TYPE_P (ttype)
+	  && vrp_val_is_max (max ())
+	  && TYPE_PRECISION (ttype) != 1)
 	fprintf (file, "+INF");
       else
 	print_generic_expr (file, max ());
@@ -398,7 +401,7 @@ value_range_base::dump (FILE *file) const
   else if (varying_p ())
     fprintf (file, "VARYING");
   else
-    fprintf (file, "INVALID RANGE");
+    gcc_unreachable ();
 }
 
 void
@@ -425,17 +428,45 @@ value_range::dump (FILE *file) const
 }
 
 void
-value_range_base::dump () const
+dump_value_range (FILE *file, const value_range *vr)
 {
-  dump_value_range (stderr, this);
-  fprintf (stderr, "\n");
+  if (!vr)
+    fprintf (file, "[]");
+  else
+    vr->dump (file);
 }
 
 void
-value_range::dump () const
+dump_value_range (FILE *file, const value_range_base *vr)
+{
+  if (!vr)
+    fprintf (file, "[]");
+  else
+    vr->dump (file);
+}
+
+DEBUG_FUNCTION void
+debug (const value_range_base *vr)
+{
+  dump_value_range (stderr, vr);
+}
+
+DEBUG_FUNCTION void
+debug (const value_range_base &vr)
+{
+  dump_value_range (stderr, &vr);
+}
+
+DEBUG_FUNCTION void
+debug (const value_range *vr)
+{
+  dump_value_range (stderr, vr);
+}
+
+DEBUG_FUNCTION void
+debug (const value_range &vr)
 {
-  dump_value_range (stderr, this);
-  fprintf (stderr, "\n");
+  dump_value_range (stderr, &vr);
 }
 
 /* Return true if the SSA name NAME is live on the edge E.  */
@@ -2165,43 +2196,6 @@ extract_range_from_unary_expr (value_range_base *vr,
   return;
 }
 
-/* Debugging dumps.  */
-
-void
-dump_value_range (FILE *file, const value_range *vr)
-{
-  if (!vr)
-    fprintf (file, "[]");
-  else
-    vr->dump (file);
-}
-
-void
-dump_value_range (FILE *file, const value_range_base *vr)
-{
-  if (!vr)
-    fprintf (file, "[]");
-  else
-    vr->dump (file);
-}
-
-/* Dump value range VR to stderr.  */
-
-DEBUG_FUNCTION void
-debug_value_range (const value_range_base *vr)
-{
-  dump_value_range (stderr, vr);
-}
-
-/* Dump value range VR to stderr.  */
-
-DEBUG_FUNCTION void
-debug_value_range (const value_range *vr)
-{
-  dump_value_range (stderr, vr);
-}
-
-
 /* Given a COND_EXPR COND of the form 'V OP W', and an SSA name V,
    create a new SSA name N and return the assertion assignment
    'N = ASSERT_EXPR <V, V OP W>'.  */
diff --git a/gcc/tree-vrp.h b/gcc/tree-vrp.h
index 348fa4f0e7b..287860399c4 100644
--- a/gcc/tree-vrp.h
+++ b/gcc/tree-vrp.h
@@ -71,9 +71,7 @@ public:
   void set_and_canonicalize (enum value_range_kind, tree, tree);
   bool zero_p () const;
   bool singleton_p (tree *result = NULL) const;
-
   void dump (FILE *) const;
-  void dump () const;
 
 protected:
   void check ();
@@ -139,7 +137,6 @@ class GTY((user)) value_range : public value_range_base
   void deep_copy (const value_range *);
   void set_and_canonicalize (enum value_range_kind, tree, tree, bitmap = NULL);
   void dump (FILE *) const;
-  void dump () const;
 
  private:
   /* Deep-copies bitmap argument.  */

On Tue, Nov 13, 2018 at 3:43 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
>
>
> On 11/13/18 3:12 AM, Richard Biener wrote:
> > On Mon, Nov 12, 2018 at 10:50 AM Aldy Hernandez <aldyh@redhat.com> wrote:
> >>
> >> I have rebased my value_range dumping patch after your value_range_base
> >> changes.
> >>
> >> I know you are not a fan of the gimple-pretty-print.c chunk, but I still
> >> think having one set of dumping code is preferable to catering to
> >> possible GC corruption while debugging.  If you're strongly opposed (as,
> >> I'm putting my foot down), I can remove it as well as the relevant
> >> pretty_printer stuff.
> >
> > I'd say we do not want to change the gimple-pretty-print.c stuff also because
> > we'll miss the leading #.  I'd rather see a simple wide-int-range class
> > wrapping the interesting bits up.  I guess I'll come up with one then ;)
>
> Ok.  Removed.
>
> >
> >> The patch looks bigger than it is because I moved all the dump routines
> >> into one place.
> >>
> >> OK?
> >>
> >> p.s. After your changes, perhaps get_range_info(const_tree, value_range
> >> &) should take a value_range_base instead?
> >
> > Yes, I missed that and am now testing this change.
>
> Thanks.
>
> >
> > Btw, the patch needs updating again (sorry).  If you leave out the
> > gimple-pretty-print.c stuff there's no requirement to use the pretty-printer
> > API, right?
>
> No need to apologize for contributing code :).  Thanks.  And yes,
> there's no need for the pretty-printer bits.
>
> I've also removed the value_range*::dump() versions with no arguments,
> as now we have an overloaded debug() for use from the debugger.
>
> Testing attached patch.

OK.

Thanks,
Richard.

> Aldy