On August 10, 2020 3:51:28 PM GMT+02:00, Jakub Jelinek via Gcc-patches <[email protected]> wrote: >On Mon, Aug 10, 2020 at 02:57:31PM +0200, Aldy Hernandez wrote: >> > I think it would be nice to see e.g. in -fdump-tree-gimple dump >> > on ipa-fnsummary.c what value_range ctors does it call for the >auto_vec and >> > if that is what we want, other than that LGTM. >> >> I see the following in the dump: >> >> evaluate_properties_for_edge() >> { >> ... >> struct auto_vec known_value_ranges; >> ... >> try >> { >> ... >> auto_vec<int_range<1>, 32>::auto_vec (&known_value_ranges); >> ... >> >> It looks like the auto_vec constructor is calling the int_range<1> >> constructor correctly: >> >> auto_vec<int_range<1>, 32>::auto_vec (struct auto_vec * const this) >> { >> ... >> int_range<1>::int_range (D.130911); >> >> >> The int_range<1> constructor also looks correct: >> >> int_range<1>::int_range (struct int_range * const this, const struct >> int_range & other) >> { >> *this = {CLOBBER}; >> { >> _1 = &this->D.92184; >> _2 = &this->m_ranges; >> irange::irange (_1, _2, 1); >> _3 = &this->D.92184; >> _4 = &other->D.92184; >> irange::operator= (_3, _4); >> } >> } >> >> If y'all agree, I will push the patch with Jakub's suggested changes. > >I believe even with that change vec doesn't work properly with >(arbitrary) >non-PODs that need non-trivial construction or destruction. >I know you haven't introduced that yourself, but just would like to >discuss >the semantics we want. > >To test, I've tried: >--- vec.c.xx 2020-01-12 11:54:38.533381424 +0100 >+++ vec.c 2020-08-10 15:33:40.600680476 +0200 >@@ -544,9 +544,34 @@ test_auto_delete_vec () > > /* Run all of the selftests within this file. */ > >+struct SSXS >+{ >+ SSXS () { __builtin_printf ("SSXS ()\n"); } >+ SSXS (const SSXS &) { __builtin_printf ("SSXS (const SSXS &)\n"); } >+ SSXS &operator= (const SSXS &) { __builtin_printf ("operator= (const >SSXS &)\n"); } >+ ~SSXS () { __builtin_printf ("~SSXS ()\n"); } >+ int s; >+}; >+ > void > vec_c_tests () > { >+ { >+ auto_vec<SSXS, 8> ssxs; >+ __builtin_printf ("constructed\n"); >+ SSXS ssxso; >+ __builtin_printf ("ssxso\n"); >+ for (int i = 0; i < 16; i++) >+ { >+ __builtin_printf ("safe_push start\n"); >+ { >+ ssxs.safe_push (ssxso); >+ } >+ __builtin_printf ("safe_push end\n"); >+ } >+ __builtin_printf ("about to destruct\n"); >+ } >+ __builtin_printf ("destructed\n"); > test_quick_push (); > test_safe_push (); > test_truncate (); > >and I see following. Let me add comments to that. > >#So auto_vec constructor will default initialize all m_data >#elements (i.e. all the embedded objects): >SSXS () >SSXS () >SSXS () >SSXS () >SSXS () >SSXS () >SSXS () >SSXS () >constructed >#And now the extra object I've created. >SSXS () >ssxso ># Now, 8 times we invoke an assignment operator, the objects ># are already constructed, so all is good. >safe_push start >operator= (const SSXS &) >safe_push end >safe_push start >operator= (const SSXS &) >safe_push end >safe_push start >operator= (const SSXS &) >safe_push end >safe_push start >operator= (const SSXS &) >safe_push end >safe_push start >operator= (const SSXS &) >safe_push end >safe_push start >operator= (const SSXS &) >safe_push end >safe_push start >operator= (const SSXS &) >safe_push end >safe_push start >operator= (const SSXS &) >safe_push end ># Now, 9th safe_push, we ran out of room in the embedded buffer. ># vec_copy_construct will now copy construct the 8 elements ># in the new array which will be used from now on. >safe_push start >SSXS (const SSXS &) >SSXS (const SSXS &) >SSXS (const SSXS &) >SSXS (const SSXS &) >SSXS (const SSXS &) >SSXS (const SSXS &) >SSXS (const SSXS &) >SSXS (const SSXS &) ># But important note, we do not construct the object at new >m_vecdata[8], ># only call assignment operator on it. That is invalid C++, isn't it? >operator= (const SSXS &) >safe_push end >safe_push start ># And again, and again ... >operator= (const SSXS &) >safe_push end >safe_push start >operator= (const SSXS &) >safe_push end >safe_push start >operator= (const SSXS &) >safe_push end >safe_push start >operator= (const SSXS &) >safe_push end >safe_push start >operator= (const SSXS &) >safe_push end >safe_push start >operator= (const SSXS &) >safe_push end >safe_push start >operator= (const SSXS &) >safe_push end >about to destruct ># Now we destruct the 8 embedded elts in auto_vec plus the variable ># I had there, but the 16 elts of the m_vecdata array, 8 of those ># were copy constructed and then operator= modified, while ># the other 8 were only operator= set, are not destructed ever. >~SSXS () >~SSXS () >~SSXS () >~SSXS () >~SSXS () >~SSXS () >~SSXS () >~SSXS () >~SSXS () >destructed > >So, for vec on types with non-trivial construction and/or destruction, >the >question is, do we want to construct the elements whenever underlying >storage for those is allocated (i.e. as now let the embedded buffer >have >constructed elements and whenever we allocate new memory for the >vector, >next to the vec_copy_construct where we copy construct the oldsize >elts, >we also default construct the remaining ones) and then destruct when >deallocating the memory and then just use assignment operator to push >stuff >in, or do we want the memory uninitialized instead and only copy >construct >during quick_push and destruct during pop etc., i.e. only the currently >active elts are constructed and nothing else? E.g. auto_vec m_data >would >then need to be a different type (e.g. char array with appropriate >alignas >and size) which wouldn't need non-trivial construction and destruction. > >For types with trivial construction/destruction, I'd like to keep >current >behavior...
We want to construct / destruct on push/pop, not on allocation. We also want to use std::move upon reallocation (but then we can't use realloc, can we?) Richard. > Jakub
