On Fri, Aug 7, 2020 at 9:57 AM Jonathan Wakely <jwak...@redhat.com> wrote:
>
> 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.
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 :/
Richard.
>
>
