On Thu, 28 Sept 2023 at 10:17, Jakub Jelinek <ja...@redhat.com> wrote:
>
> Hi!
>
> On Wed, Sep 27, 2023 at 12:46:45PM +0200, Jakub Jelinek wrote:
> > On Wed, Sep 27, 2023 at 07:17:22AM +0000, Richard Biener wrote:
> > > OK I guess. Can you summarize the limitations for non-POD types
> > > in the big comment at the start of vec.h?
> >
> > Still haven't done that, but will do after we flesh out the details
> > below.
> >
> > > (can we put in static_asserts
> > > in the places that obviously do not work?)
> >
> > I've tried to do this though, I think the static_asserts will allow
> > making sure we only use what is supportable and will serve better than
> > any kind of comment.
>
> The following patch adds the file comment, as discussed on IRC adds an
> exception for qsort/sort/stablesort such that std::pair of 2 trivially
> copyable types is also allowed, and fixes some of the grow vs. grow_cleared
> issues (on top of the bitmap_head_pod patch far more), but still not all
> yet, so I've kept that static_assert for now commented out. Richard
> Sandiford said he's playing with poly_int_pod vs. poly_int and I'll resolve
> the remaining stuff incrementally afterwards plus enable the assert.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2023-09-28 Jakub Jelinek <ja...@redhat.com>
> Jonathan Wakely <jwak...@redhat.com>
>
> * vec.h: Mention in file comment limited support for non-POD types
> in some operations.
> (vec_destruct): New function template.
> (release): Use it for non-trivially destructible T.
> (truncate): Likewise.
> (quick_push): Perform a placement new into slot
> instead of assignment.
> (pop): For non-trivially destructible T return void
> rather than T & and destruct the popped element.
> (quick_insert, ordered_remove): Note that they aren't suitable
> for non-trivially copyable types. Add static_asserts for that.
> (block_remove): Assert T is trivially copyable.
> (vec_detail::is_trivially_copyable_or_pair): New trait.
> (qsort, sort, stablesort): Assert T is trivially copyable or
> std::pair with both trivally copyable types.
> (quick_grow): Add assert T is trivially default constructible,
> for now commented out.
> (quick_grow_cleared): Don't call quick_grow, instead inline it
> by hand except for the new static_assert.
> (gt_ggc_mx): Assert T is trivially destructable.
> (auto_vec::operator=): Formatting fixes.
> (auto_vec::auto_vec): Likewise.
> (vec_safe_grow_cleared): Don't call vec_safe_grow, instead inline
> it manually and call quick_grow_cleared method rather than quick_grow.
> (safe_grow_cleared): Likewise.
> * edit-context.cc (class line_event): Move definition earlier.
> * tree-ssa-loop-im.cc (seq_entry::seq_entry): Make default ctor
> defaulted.
> * ipa-fnsummary.cc (evaluate_properties_for_edge): Use
> safe_grow_cleared instead of safe_grow followed by placement new
> constructing the elements.
>
> --- gcc/vec.h.jj 2023-09-27 10:38:50.635845540 +0200
> +++ gcc/vec.h 2023-09-28 11:05:14.776215137 +0200
> @@ -111,6 +111,24 @@ extern void *ggc_realloc (void *, size_t
> the 'space' predicate will tell you whether there is spare capacity
> in the vector. You will not normally need to use these two functions.
>
> + Not all vector operations support non-POD types and such restrictions
> + are enforced through static assertions. Some operations which often use
> + memmove to move elements around like quick_insert, safe_insert,
> + ordered_remove, unordered_remove, block_remove etc. require trivially
> + copyable types. Sorting operations, qsort, sort and stablesort, require
> + those too but as an extension allow also std::pair of 2 trivially copyable
> + types which happens to work even when std::pair itself isn't trivially
> + copyable. The quick_grow and safe_grow operations require trivially
> + default constructible types. One can use quick_grow_cleared or
> + safe_grow_cleared for non-trivially default constructible types if needed
> + (but of course such operation is more expensive then). The pop operation
> + returns reference to the last element only for trivially destructible
> + types, for non-trivially destructible types one should use last operation
> + followed by pop which in that case returns void.
> + And finally, the GC and GC atomic vectors should always be used with
> + trivially destructible types, as nothing will invoke destructors when they
> + are freed during GC.
> +
> Notes on the different layout strategies
>
> * Embeddable vectors (vec<T, A, vl_embed>)
> @@ -185,6 +203,16 @@ extern void dump_vec_loc_statistics (voi
> /* Hashtable mapping vec addresses to descriptors. */
> extern htab_t vec_mem_usage_hash;
>
> +/* Destruct N elements in DST. */
> +
> +template <typename T>
> +inline void
> +vec_destruct (T *dst, unsigned n)
> +{
> + for ( ; n; ++dst, --n)
> + dst->~T ();
> +}
> +
> /* Control data for vectors. This contains the number of allocated
> and used slots inside a vector. */
>
> @@ -310,6 +338,9 @@ va_heap::release (vec<T, va_heap, vl_emb
> if (v == NULL)
> return;
>
> + if (!std::is_trivially_destructible <T>::value)
Do GCC's coding standards really require a space before the template
argument list, like for a function parameter list?
That style doesn't seem to be used elsewhere (and is not idiomatic for
C++ at all).
> +namespace vec_detail
> +{
> + /* gcc_{qsort,qsort_r,stablesort_r} implementation under the hood
> + uses memcpy/memmove to reorder the array elements.
> + We want to assert these methods aren't used on types for which
> + that isn't appropriate, but unfortunately std::pair of 2 trivially
> + copyable types isn't trivially copyable and we use qsort on many
> + such std::pair instantiations. Let's allow both trivially
> + copyable types and std::pair of 2 trivially copyable types as
> + exception for qsort/sort/stablesort. */
> + template<typename T>
> + struct is_trivially_copyable_or_pair : std::is_trivially_copyable<T> { };
> +
> + template<typename T, typename U>
> + struct is_trivially_copyable_or_pair<std::pair<T, U> >
The space in "> >" is only required in C++98, we don't need it in C++11.
> + : std::integral_constant<bool, std::is_trivially_copyable<T>::value
> + && std::is_trivially_copyable<U>::value> { };
> +}
> +
