std:vec for classes with constructor?
Richard Biener
richard.guenther@gmail.com
Fri Aug 7 08:54:04 GMT 2020
On Fri, Aug 7, 2020 at 10:34 AM Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On 07/08/20 10:22 +0200, Richard Biener wrote:
> >On Fri, Aug 7, 2020 at 9:57 AM Jonathan Wakely <jwakely@redhat.com> wrote:
> >>
> >> On 07/08/20 08:48 +0200, Richard Biener wrote:
> >> >On Thu, Aug 6, 2020 at 9:24 PM Jonathan Wakely <jwakely@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
> >> >> >>>>><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 ();
> >> >> >>>>+ }
> >> >> >>>> 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.
> >
> >Now that you say it, vec has a T[1] member so depending on T
> >there might be a side-effect as invoking its CTOR? Or it might
> >even not compile if there is no default CTOR available... Ick :/
>
> Right.
>
> Does the GTY stuff add members to the struct, or otherwise alter its
> layout?
No.
> If not, then I'd just assume that the m_vecdata offset is
> sizeof(vec_prefix), and add a static assert in the vec constructor
> (where you do have an object to take the address of) to verify it. If
> there is some target where the assumption isn't true the static assert
> will fail as soon as you try to use the type, and we can fix it then.
The point of the exercise is to factor in the effects of alignment of T
on the m_vecdata member offset. Just using sizeof(vec_prefix)
doesn't work there. Of course ROUND_UP (sizeof (vec_prefix), alignof (T))
might work. But then it's all less portable than what offsetof() intended to be
and we do know we're not dealing with any virtual inheritance or so thus
the FE telling us that offsetof is non-standard here is unhelpful :/
Now could we just use an uninitialized pointer / pointer to an
uninitialized object here?
vec *tem; // = XALLOCA (vec);
return (char *)&tem->m_vecdata - (char *)tem + alloc * sizeof (T);
it's going to look awfully like the old-school NULL pointer offsetof
implementation though.
Richard.
>
More information about the Gcc-patches
mailing list