Re: [PATCH] Convert ipcp_vr_lattice to type agnostic framework.

[Aldy Hernandez <aldyh@redhat.com>]

My apologies for the delay.  I was on vacation.

On 5/26/23 18:17, Martin Jambor wrote:
> Hello,
> 
> On Mon, May 22 2023, Aldy Hernandez wrote:
>> I've adjusted the patch with some minor cleanups that came up when I
>> implemented the rest of the IPA revamp.
>>
>> Rested.  OK?
>>
>> On Wed, May 17, 2023 at 4:31 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>>
>>> This converts the lattice to store ranges in Value_Range instead of
>>> value_range (*) to make it type agnostic, and adjust all users
>>> accordingly.
>>>
>>> I think it is a good example on converting from static ranges to more
>>> general, type agnostic ones.
>>>
>>> I've been careful to make sure Value_Range never ends up on GC, since
>>> it contains an int_range_max and can expand on-demand onto the heap.
>>> Longer term storage for ranges should be done with vrange_storage, as
>>> per the previous patch ("Provide an API for ipa_vr").
>>>
>>> (*) I do know the Value_Range naming versus value_range is quite
>>> annoying, but it was a judgement call last release for the eventual
>>> migration to having "value_range" be a type agnostic range object.  We
>>> will ultimately rename Value_Range to value_range.
> 
> It is quite confusing for an unsuspecting reader indeed.
> 
>>>
>>> OK for trunk?
> 
> I guess I need to rely on that you know what you are doing :-)
> I have seen in other messages that you measure the compile time
> effects of your patches, do you look at memory use as well?

Before going in depth into the rest of your review (thanks BTW), let's 
address memory usage.

To be honest, I didn't measure memory, but only because I had a pretty 
good inkling that it wouldn't make a difference, since vrange_storage is 
designed to take less space than value_range which you were using.  But 
you're right, I should've measured it.

First value_range is a derived-POD so it has a vtable pointer in there. 
vrange_storage does not.

Second, vrange_storage only uses the minimum number of bytes to 
represent the integers in the range.  vrange_storage uses a 
trailing_wide_int type mechanism to store the minimum amount of HWIs for 
a range.   See irange_storage::set_irange().

value_range is a typedef for int_range<2>.  Storing this in GC, which 
IPA currently does, takes 432 bytes.  I will be removing the GTY markers 
for irange, to keep anyone from even trying.

On the other hand, storing this same range in GC memory with 
vrange_storage takes 35 bytes for a [5, 10] range:

   unsigned prec = TYPE_PRECISION (integer_type_node);
   wide_int min = wi::shwi (5, prec);
   wide_int max = wi::shwi (10, prec);
   int_range<2> r (integer_type_node, min, max);
   vrange_storage *p = ggc_alloc_vrange_storage (r);

Breaking on irange_storage::alloc() you can see the size of the 
allocated object is 35 bytes.  This is far less than 432 bytes in trunk 
(due to the wide_int penalty in this release), but even so is comparable 
to GCC12 which took 32 bytes.  Note that GCC12 was using trees, so those 
32 bytes were deceptive, since there was another level of indirection 
involved for the actual HWIs.

Anywhoo... I tried comparing GCC 12 to current mainline plus these 
patches, but it was a royal PITA, because so much has changed, not just 
in the ranger world, but in the rest of the compiler.

However, comparing trunk against my patches is a total wash, even 
considering that IPA will now store a gazillion more ranges.

For measuring I built with --enable-gather-detailed-mem-stats and 
compared the "Total Allocated:" lines from -fmem-report for the 
aggregate of all .ii files in a bootstrap:

Before:
Total allocated: 73360474112.0 bytes

After:
Total allocated: 73354182656.0 bytes

So we use 0.00858% less memory.

To be honest, this is an unfair comparison because trunk IPA is 
streaming out the full value_range (wide_int's and all which changed in 
this release), but with these patches:

a) We don't stream out vtable pointer.
b) Even if the number of sub-ranges can be larger, we only store the 
bare minimum, and we're capped at 255 sub-ranges (which I've never seen 
in the wild).

I think we're good, but if this ever becomes a problem, the constructor 
for ipa_vr could just be tweaked to squish things down before allocating:

ipa_vr::ipa_vr (const vrange &v)
   : m_type (v.type ())
{
   if (is_a <irange> (v))
     {
       int_range<10> squish (as_a <irange> (v));
       m_storage = ggc_alloc_vrange_storage (squish);
     }
   else
     m_storage = ggc_alloc_vrange_storage (v);
}

I'll address the rest of your comments in follow-up mails.
Aldy