From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 1865E3858C78 for ; Fri, 29 Sep 2023 10:00:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1865E3858C78 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1695981616; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=tjmtXQ5zHgOqZzc58jdrIPyXR7NOWc5piKJ6438U15I=; b=TCfAqmU/vcyB1Dgpzg2FgWDjttr4KpSgydMhKm/vVoL6DJ9SD9Z3s+fVREjIt3pkQ/9z4E 8JHxmDr1Y+gxek1xerp0aON3E8KDc24AXK7VNN7Yaa2sz190pOebx6pyjajDw63y1iDgkJ hwzM2CEdRkjrn7puOU7pygDQxX/VcHs= Received: from mail-lj1-f199.google.com (mail-lj1-f199.google.com [209.85.208.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-614-LzaC9o2hNZuDlosZ4NjZvg-1; Fri, 29 Sep 2023 06:00:14 -0400 X-MC-Unique: LzaC9o2hNZuDlosZ4NjZvg-1 Received: by mail-lj1-f199.google.com with SMTP id 38308e7fff4ca-2bff6dcbccaso199962161fa.1 for ; Fri, 29 Sep 2023 03:00:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695981613; x=1696586413; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=tjmtXQ5zHgOqZzc58jdrIPyXR7NOWc5piKJ6438U15I=; b=rNQCPU9SJpqWyToEeCZDJnY670epA/L8COBvq0z0Ha8hxC9rs74kJtAAwKEam5v900 aHMyR4rwAQieZE3t0xpoQgEjtAC1yRDEROx//v4vor8kZ71cpcLl0i0UjTqksCPChvgO J4F/TJzDbwhsh6DPXXgcVaOC0MIm/PixptHBgAsItey/CcbIoeYt90O+nQMeVxH44WMI 4HL8IU1+vUJDS75wP1cnt3VJWsOS/efpSChsjBUzsgtSvHtUmgtCk7UaIxSVA7faJzyK OfOzU6vbDYcgo5ytAKffveS91B5oDv/+SvD4ErWNOk1duRkY4/yovBfy5tYulpevWN6q U7qQ== X-Gm-Message-State: AOJu0Yzre8KDAiBGhrRbrAlsWiCHs3cgx/g9NTaoJiyiLyxxr68bP7/n xWph5k/z8tkwKPtGPrvr71+UTnY42qb4m5j7+N90yOCIOt73aEeG9ftB4hIxsNFfs7hAueEsX+w 1zAUDKce1pMhtxCYRrvkA/OJHipUegYZXxw== X-Received: by 2002:a2e:8505:0:b0:2bc:b9c7:7ba3 with SMTP id j5-20020a2e8505000000b002bcb9c77ba3mr3344574lji.12.1695981613196; Fri, 29 Sep 2023 03:00:13 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEAF51rZdQK8K4ueCvLpjIozhPEW/fyCrT2fe5p7VH/qmDxtG0VXOfuMCL8m/MM27RUlSC4OTzYEZnq0CBM/GM= X-Received: by 2002:a2e:8505:0:b0:2bc:b9c7:7ba3 with SMTP id j5-20020a2e8505000000b002bcb9c77ba3mr3344551lji.12.1695981612810; Fri, 29 Sep 2023 03:00:12 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Jonathan Wakely Date: Fri, 29 Sep 2023 11:00:01 +0100 Message-ID: Subject: Re: [PATCH] vec.h, v3: Make some ops work with non-trivially copy constructible and/or destructible types To: Jakub Jelinek Cc: Richard Biener , Richard Sandiford , gcc-patches@gcc.gnu.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-6.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Thu, 28 Sept 2023 at 10:17, Jakub Jelinek 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 > Jonathan Wakely > > * 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) > @@ -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 > +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 if (v == NULL) > return; > > + if (!std::is_trivially_destructible ::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 > + struct is_trivially_copyable_or_pair : std::is_trivially_copyable { }; > + > + template > + struct is_trivially_copyable_or_pair > The space in "> >" is only required in C++98, we don't need it in C++11. > + : std::integral_constant::value > + && std::is_trivially_copyable::value> { }; > +} > +