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 >> wrote: >>> >>> On 06/08/20 06:16 +0100, Richard Sandiford wrote: >>> >Andrew MacLeod via Gcc-patches 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 >>> wrote: >>> >>>> On Fri, Jul 31 2020, Aldy Hernandez via Gcc-patches wrote: >>> >>>> [...] >>> >>>> >>> >>>>> * ipa-cp changes from vec to std::vec. >>> >>>>> >>> >>>>> 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 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 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::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*&, unsigned int, bool) [with T = int_range<1>]’ > /home/aldyh/src/gcc/gcc/vec.h:1746:20: required from ‘bool vec::reserve(unsigned int, bool) [with T = int_range<1>]’ > /home/aldyh/src/gcc/gcc/vec.h:1766:10: required from ‘bool vec::reserve_exact(unsigned int) [with T = int_range<1>]’ > /home/aldyh/src/gcc/gcc/vec.h:1894:3: required from ‘void vec::safe_grow(unsigned int) [with T = int_range<1>]’ > /home/aldyh/src/gcc/gcc/vec.h:1912:3: required from ‘void vec::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, 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? Aldy