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;
vec_embedded tem;
return (char *)&tem.m_vecdata - (char *)&tem + alloc * sizeof (T);
}
?
