From: Richard Biener <richard.guenther@gmail.com>
To: Jonathan Wakely <jwakely@redhat.com>
Cc: Aldy Hernandez <aldyh@redhat.com>,
Andrew MacLeod via Gcc-patches <gcc-patches@gcc.gnu.org>,
Martin Jambor <mjambor@suse.cz>, Jeff Law <law@redhat.com>,
Andrew MacLeod <amacleod@redhat.com>,
Richard Sandiford <richard.sandiford@arm.com>
Subject: Re: std:vec for classes with constructor?
Date: Fri, 7 Aug 2020 10:22:34 +0200 [thread overview]
Message-ID: <CAFiYyc2fKqU4tYcxudrX45eJ=3qN0uac8yptfY1fakb1CO0tFw@mail.gmail.com> (raw)
In-Reply-To: <20200807075746.GK3400@redhat.com>
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 :/
Richard.
>
>
next prev parent reply other threads:[~2020-08-07 8:22 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-31 21:44 [patch] multi-range implementation for value_range (irange) Aldy Hernandez
2020-08-05 14:27 ` Gerald Pfeifer
2020-08-05 15:45 ` Aldy Hernandez
2020-08-05 22:43 ` Gerald Pfeifer
2020-08-06 4:00 ` Richard Sandiford
2020-08-10 3:42 ` Martin Liška
2020-08-10 7:44 ` Aldy Hernandez
2020-08-10 10:22 ` Gerald Pfeifer
2020-08-10 10:30 ` Martin Liška
2020-08-10 11:03 ` Aldy Hernandez
2020-08-05 15:09 ` std:vec for classes with constructor? (Was: Re: [patch] multi-range implementation for value_range (irange)) Martin Jambor
2020-08-05 15:41 ` Aldy Hernandez
2020-08-05 16:55 ` Richard Biener
2020-08-24 21:53 ` Jeff Law
2020-08-24 22:03 ` Andrew MacLeod
2020-08-05 16:54 ` Richard Biener
2020-08-06 1:07 ` Andrew MacLeod
2020-08-06 5:16 ` std:vec for classes with constructor? Richard Sandiford
2020-08-06 10:19 ` Jonathan Wakely
2020-08-06 10:31 ` Richard Biener
2020-08-06 10:48 ` Jonathan Wakely
2020-08-06 14:17 ` Aldy Hernandez
2020-08-06 14:35 ` Richard Biener
2020-08-06 14:59 ` Aldy Hernandez
2020-08-06 16:30 ` Jonathan Wakely
2020-08-06 17:58 ` Aldy Hernandez
2020-08-06 19:24 ` Jonathan Wakely
2020-08-07 6:48 ` Richard Biener
2020-08-07 7:57 ` Jonathan Wakely
2020-08-07 8:22 ` Richard Biener [this message]
2020-08-07 8:34 ` Jonathan Wakely
2020-08-07 8:54 ` Richard Biener
2020-08-07 8:55 ` Jakub Jelinek
2020-08-07 9:17 ` Jonathan Wakely
2020-08-07 18:04 ` Aldy Hernandez
2020-08-07 18:33 ` Jakub Jelinek
2020-08-10 12:57 ` Aldy Hernandez
2020-08-10 13:05 ` Aldy Hernandez
2020-08-10 18:03 ` Jakub Jelinek
2020-08-10 13:51 ` Jakub Jelinek
2020-08-10 16:58 ` Richard Biener
2020-08-10 17:18 ` Jakub Jelinek
2020-08-13 11:38 ` r11-2663 causes static_assert failure (was: Re: std:vec for classes with constructor?) Tobias Burnus
2020-08-13 11:52 ` Jakub Jelinek
2020-08-13 12:06 ` r11-2663 causes static_assert failure Tobias Burnus
2020-08-13 12:25 ` Jakub Jelinek
2020-08-13 12:40 ` Jakub Jelinek
2020-08-13 12:46 ` Iain Sandoe
2020-08-13 12:55 ` Iain Sandoe
2020-08-13 13:04 ` Jakub Jelinek
2020-08-13 13:00 ` Jakub Jelinek
2020-08-06 6:57 ` std:vec for classes with constructor? (Was: Re: [patch] multi-range implementation for value_range (irange)) Richard Biener
2020-08-06 10:23 ` Jonathan Wakely
2020-08-06 11:03 ` Aldy Hernandez
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFiYyc2fKqU4tYcxudrX45eJ=3qN0uac8yptfY1fakb1CO0tFw@mail.gmail.com' \
--to=richard.guenther@gmail.com \
--cc=aldyh@redhat.com \
--cc=amacleod@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jwakely@redhat.com \
--cc=law@redhat.com \
--cc=mjambor@suse.cz \
--cc=richard.sandiford@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).