cleanups and unification of value_range dumping code

[Aldy Hernandez]

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