On 07/08/20 08:48 +0200, Richard Biener wrote:
>On Thu, Aug 6, 2020 at 9:24 PM Jonathan Wakely <jwak...@redhat.com> wrote:
>>
>> 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.
>
>Yeah, we eventually have no object when we need the offset. Does
>offsetof refusing to operate on this mean that using a temporary
>object on the stack like the following can yield a different answer
>from the "real" object? I guess examples where it breaks boil
>down to virtual inheritance. Anyway, so the following _should_ work ...
>
>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;
This typedef is redundant, the name 'vec' in thos scope refers to the
current instantiation, so you can just say 'vec'.
> vec_embedded tem;
i.e. 'vec tem;' here, and remove the typedef on the previous line.
> return (char *)&tem.m_vecdata - (char *)&tem + alloc * sizeof (T);
>}
>
>?
Yes, that should work fine. In general you wouldn't want to create an
object just to do this, but creating and destroying a vec has no side
effects so is fine.
