On 06/08/20 19:58 +0200, Aldy Hernandez wrote:
>
>
>On 8/6/20 6:30 PM, Jonathan Wakely wrote:
>>On 06/08/20 16:17 +0200, Aldy Hernandez 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
>>>>><jwak...@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
>>>>>><mjam...@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 ();
>>>>+ }
>>>> 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);
>>>> | ^
>>
>>Can we just avoid using offsetof there?
>>
>>Untested ...
>>
>>--- a/gcc/vec.h
>>+++ b/gcc/vec.h
>>@@ -1281,8 +1281,8 @@ template<typename T, typename A>
>> inline size_t
>> vec<T, A, vl_embed>::embedded_size (unsigned alloc)
>> {
>>- typedef vec<T, A, vl_embed> vec_embedded;
>>- return offsetof (vec_embedded, m_vecdata) + alloc * sizeof (T);
>>+ size_t offset = (char*)&m_vecdata - (char*)this;
>>+ return offset + alloc * sizeof (T);
>> }
>>
>>
>
>Now we have:
>
>In file included from /home/aldyh/src/gcc/gcc/rtl.h:30,
> from /home/aldyh/src/gcc/gcc/genpreds.c:27:
>/home/aldyh/src/gcc/gcc/vec.h: In static member function ‘static
>size_t vec<T, A, vl_embed>::embedded_size(unsigned int)’:
>/home/aldyh/src/gcc/gcc/vec.h:1284:27: error: invalid use of member
>‘vec<T, A, vl_embed>::m_vecdata’ in static member function
> 1284 | size_t offset = (char*)&m_vecdata - (char*)this;
> | ^~~~~~~~~
>/home/aldyh/src/gcc/gcc/vec.h:626:5: note: declared here
> 626 | T m_vecdata[1];
> | ^~~~~~~~~
>/home/aldyh/src/gcc/gcc/vec.h:1284:46: error: ‘this’ is unavailable
>for static member functions
> 1284 | size_t offset = (char*)&m_vecdata - (char*)this;
> | ^~~~
>
>
Oh sorry, I didn't realise it was static.
