On Thu, Aug 6, 2020 at 4:17 PM Aldy Hernandez <al...@redhat.com> 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 ();
With your placement new loop you should only need .safe_grow (count)
which should then make it work(?)
> > + }
> > 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);
> > | ^
>
> The attached patch reverts the functionality to auto_vec<>, but the
> in-place new doesn't work. Does anyone have any suggestions?
I'm not able to decipher the above but I wonder why we have vl_embed
here. Does the error vanish
if you use auto_vec<value_range> instead of auto_vec<value_range, 32>
here? I guess the issue
is
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);
}
when T is not a std layout type even though its offset does not depend on the
layout of T ... The record in question is
template<typename T, typename A>
struct GTY((user)) vec<T, A, vl_embed>
{
public:
...
/* FIXME - These fields should be private, but we need to cater to
compilers that have stricter notions of PODness for types. */
vec_prefix m_vecpfx;
T m_vecdata[1];
};
and the intent is to count tail-padding for say 'char' where sizeof
(vec<>) would
cover 4 elements already.
Richard.
>
> Aldy
