std:vec for classes with constructor?

[Aldy Hernandez]

On 8/6/20 4:35 PM, Richard Biener wrote:
> On Thu, Aug 6, 2020 at 4:17 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>
>>
>>
>> On 8/6/20 12:48 PM, Jonathan Wakely wrote:
>>> On 06/08/20 12:31 +0200, Richard Biener wrote:
>>>> On Thu, Aug 6, 2020 at 12:19 PM Jonathan Wakely <jwakely@redhat.com>
>>>> wrote:
>>>>>
>>>>> On 06/08/20 06:16 +0100, Richard Sandiford wrote:
>>>>>> Andrew MacLeod via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>>>>>>> On 8/5/20 12:54 PM, Richard Biener via Gcc-patches wrote:
>>>>>>>> On August 5, 2020 5:09:19 PM GMT+02:00, Martin Jambor
>>>>> <mjambor@suse.cz> wrote:
>>>>>>>>> On Fri, Jul 31 2020, Aldy Hernandez via Gcc-patches wrote:
>>>>>>>>> [...]
>>>>>>>>>
>>>>>>>>>> * ipa-cp changes from vec<value_range> to std::vec<value_range>.
>>>>>>>>>>
>>>>>>>>>> We are using std::vec to ensure constructors are run, which they
>>>>>>>>> aren't
>>>>>>>>>> in our internal vec<> implementation.  Although we usually
>>>>> steer away
>>>>>>>>>> from using std::vec because of interactions with our GC system,
>>>>>>>>>> ipcp_param_lattices is only live within the pass and allocated
>>>>> with
>>>>>>>>> calloc.
>>>>>>>>> Ummm... I did not object but I will save the URL of this message
>>>>> in the
>>>>>>>>> archive so that I can waive it in front of anyone complaining why I
>>>>>>>>> don't use our internal vec's in IPA data structures.
>>>>>>>>>
>>>>>>>>> But it actually raises a broader question: was this supposed to
>>>>> be an
>>>>>>>>> exception, allowed only not to complicate the irange patch
>>>>> further, or
>>>>>>>>> will this be generally accepted thing to do when someone wants
>>>>> to have
>>>>>>>>> a
>>>>>>>>> vector of constructed items?
>>>>>>>> It's definitely not what we want. You have to find another
>>>>> solution to this problem.
>>>>>>>>
>>>>>>>> Richard.
>>>>>>>>
>>>>>>>
>>>>>>> Why isn't it what we want?
>>>>>>>
>>>>>>> This is a small vector local to the pass so it doesn't interfere with
>>>>>>> our PITA GTY.
>>>>>>> The class is pretty straightforward, but we do need a constructor to
>>>>>>> initialize the pointer and the max-size field.  There is no
>>>>> allocation
>>>>>>> done per element, so a small number of elements have a couple of
>>>>> fields
>>>>>>> initialized per element. We'd have to loop to do that anyway.
>>>>>>>
>>>>>>> GCC's vec<> does not provide he ability to run a constructor,
>>>>> std::vec
>>>>>>> does.
>>>>>>
>>>>>> I realise you weren't claiming otherwise, but: that could be fixed :-)
>>>>>
>>>>> It really should be.
>>>>>
>>>>> Artificial limitations like that are just a booby trap for the unwary.
>>>>
>>>> It's probably also historic because we couldn't even implement
>>>> the case of re-allocation correctly without std::move, could we?
>>>
>>> I don't see why not. std::vector worked fine without std::move, it's
>>> just more efficient with std::move, and can be used with a wider set
>>> of element types.
>>>
>>> When reallocating you can just copy each element to the new storage
>>> and destroy the old element. If your type is non-copyable then you
>>> need std::move, but I don't think the types I see used with vec<> are
>>> non-copyable. Most of them are trivially-copyable.
>>>
>>> I think the benefit of std::move to GCC is likely to be permitting
>>> cheap copies to be made where previously they were banned for
>>> performance reasons, but not because those copies were impossible.
>>
>> For the record, neither value_range nor int_range<N> require any
>> allocations.  The sub-range storage resides in the stack or wherever it
>> was defined.  However, it is definitely not a POD.
>>
>> Digging deeper, I notice that the original issue that caused us to use
>> std::vector was not in-place new but the safe_grow_cleared.  The
>> original code had:
>>
>>>        auto_vec<value_range, 32> known_value_ranges;
>>> ...
>>> ...
>>>        if (!vr.undefined_p () && !vr.varying_p ())
>>>            {
>>>              if (!known_value_ranges.length ())
>>>                known_value_ranges.safe_grow_cleared (count);
>>>                known_value_ranges[i] = vr;
>>>            }
>>
>> I would've gladly kept the auto_vec, had I been able to do call the
>> constructor by doing an in-place new:
>>
>>>                      if (!vr.undefined_p () && !vr.varying_p ())
>>>                        {
>>>                          if (!known_value_ranges.length ())
>>> -                         known_value_ranges.safe_grow_cleared (count);
>>> +                         {
>>> +                           known_value_ranges.safe_grow_cleared (count);
>>> +                           for (int i = 0; i < count; ++i)
>>> +                             new (&known_value_ranges[i]) value_range ();
> 
> With your placement new loop you should only need .safe_grow (count)
> which should then make it work(?)

Ah yes, no cleared is necessary as the in-place new would initialize the 
chunk.  But as discussed below, the compiler still barfs.

> 
>>> +                         }
>>>                          known_value_ranges[i] = vr;
>>>                        }
>>>                    }
>>
>> But alas, compiling yields:
>>
>>> In file included from /usr/include/wchar.h:35,
>>>                   from /usr/include/c++/10/cwchar:44,
>>>                   from /usr/include/c++/10/bits/postypes.h:40,
>>>                   from /usr/include/c++/10/iosfwd:40,
>>>                   from /usr/include/gmp-x86_64.h:34,
>>>                   from /usr/include/gmp.h:59,
>>>                   from /home/aldyh/src/gcc/gcc/system.h:687,
>>>                   from /home/aldyh/src/gcc/gcc/ipa-fnsummary.c:55:
>>> /home/aldyh/src/gcc/gcc/vec.h: In instantiation of ‘static size_t vec<T, A, vl_embed>::embedded_size(unsigned int) [with T = int_range<1>; A = va_heap; size_t = long unsigned int]’:
>>> /home/aldyh/src/gcc/gcc/vec.h:288:58:   required from ‘static void va_heap::reserve(vec<T, va_heap, vl_embed>*&, unsigned int, bool) [with T = int_range<1>]’
>>> /home/aldyh/src/gcc/gcc/vec.h:1746:20:   required from ‘bool vec<T>::reserve(unsigned int, bool) [with T = int_range<1>]’
>>> /home/aldyh/src/gcc/gcc/vec.h:1766:10:   required from ‘bool vec<T>::reserve_exact(unsigned int) [with T = int_range<1>]’
>>> /home/aldyh/src/gcc/gcc/vec.h:1894:3:   required from ‘void vec<T>::safe_grow(unsigned int) [with T = int_range<1>]’
>>> /home/aldyh/src/gcc/gcc/vec.h:1912:3:   required from ‘void vec<T>::safe_grow_cleared(unsigned int) [with T = int_range<1>]’
>>> /home/aldyh/src/gcc/gcc/ipa-fnsummary.c:634:51:   required from here
>>> /home/aldyh/src/gcc/gcc/vec.h:1285:20: warning: ‘offsetof’ within non-standard-layout type ‘vec_embedded’ {aka ‘vec<int_range<1>, va_heap, vl_embed>’} is conditionally-supported [-Winvalid-offsetof]
>>>   1285 |   return offsetof (vec_embedded, m_vecdata) + alloc * sizeof (T);
>>>        |                    ^
>>
>> The attached patch reverts the functionality to auto_vec<>, but the
>> in-place new doesn't work.  Does anyone have any suggestions?
> 
> I'm not able to decipher the above but I wonder why we have vl_embed
> here.  Does the error vanish
> if you use auto_vec<value_range> instead of auto_vec<value_range, 32>
> here?  I guess the issue

No.

Aldy