From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) by sourceware.org (Postfix) with ESMTPS id 2AD103858D32 for ; Mon, 29 Aug 2022 11:50:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2AD103858D32 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=suse.cz Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id E90AA1F86C; Mon, 29 Aug 2022 11:50:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1661773801; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=dE7uTTHH7+EPPK0Mu8IAsV2O5LoiXtjh1xk8mexSEr4=; b=e2XC+zH/zvBZA4ShQI02n63OFy6eulfhFuTR+dy3rQ5Q6UnafLbQdFQj41aIgIvcvoH+q0 HquvUsrzSS8ecJ2DRyxo9VOEU+BEy3lpfjx0Hlke8Q8gqBBQ2WndHk1cOouNQpq5PdrvBV 07f6ZKQXgRND4akVmLlcPtgB3eRojPY= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1661773801; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=dE7uTTHH7+EPPK0Mu8IAsV2O5LoiXtjh1xk8mexSEr4=; b=36r/HaxL8adVR+tDtV6lAr1aAM9td5eywjpPOlmPFUdbhu16D527MtUl2NkAPSJLU2lnk/ TQN9xLIaWhBP8hCw== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id D3C981352A; Mon, 29 Aug 2022 11:50:01 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id aFaHM+mnDGO8JAAAMHmgww (envelope-from ); Mon, 29 Aug 2022 11:50:01 +0000 From: Martin Jambor To: Richard Biener Cc: GCC Patches , Richard Sandiford Subject: Re: [PATCH 1/2] vec: Add array_slice constructors from non-const and gc vectors In-Reply-To: References: User-Agent: Notmuch/0.35 (https://notmuchmail.org) Emacs/28.1 (x86_64-suse-linux-gnu) Date: Mon, 29 Aug 2022 13:50:01 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_NONE,SPF_SOFTFAIL,TXREP,T_SCC_BODY_TEXT_LINE 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: Hi, On Mon, Aug 29 2022, Richard Biener wrote: > On Mon, 29 Aug 2022, Martin Jambor wrote: > >> Hi again, >>=20 >> On Mon, Aug 29 2022, Richard Biener wrote: >> > On Fri, 26 Aug 2022, Martin Jambor wrote: >> > >> >> Hi, >> >>=20 >> >> On Fri, Aug 26 2022, Richard Biener wrote: >> >> >> Am 26.08.2022 um 18:39 schrieb Martin Jambor : >> >> >> >> >> >> =EF=BB=BFHi, >> >> >> >> >> >> This patch adds constructors of array_slice that are required to >> >> >> create them from non-const (heap or auto) vectors or from GC vecto= rs. >> >> >> >> >> >> The use of non-const array_slices is somewhat limited, as creating= one >> >> >> from const vec still leads to array_slice, >> >> >> so I eventually also only resorted to having read-only array_slice= s. >> >> >> But I do need the constructor from the gc vector. >> >> >> >> >> >> Bootstrapped and tested along code that actually uses it on >> >> >> x86_64-linux. OK for trunk? >> >> >> >> >> >> Thanks, >> >> >> >> >> >> Martin >> >> >> >> >> >> >> >> >> gcc/ChangeLog: >> >> >> >> >> >> 2022-08-08 Martin Jambor >> >> >> >> >> >> * vec.h (array_slice): Add constructors for non-const reference= to >> >> >> heap vector and pointers to heap vectors. >> >> >> --- >> >> >> gcc/vec.h | 12 ++++++++++++ >> >> >> 1 file changed, 12 insertions(+) >> >> >> >> >> >> diff --git a/gcc/vec.h b/gcc/vec.h >> >> >> index eed075addc9..b0477e1044c 100644 >> >> >> --- a/gcc/vec.h >> >> >> +++ b/gcc/vec.h >> >> >> @@ -2264,6 +2264,18 @@ public: >> >> >> array_slice (const vec &v) >> >> >> : m_base (v.address ()), m_size (v.length ()) {} >> >> >> >> >> >> + template >> >> >> + array_slice (vec &v) >> >> >> + : m_base (v.address ()), m_size (v.length ()) {} >> >> >> + >> >> >> + template >> >> >> + array_slice (const vec *v) >> >> >> + : m_base (v ? v->address () : nullptr), m_size (v ? v->length= () : 0) {} >> >> >> + >> >> >> + template >> >> >> + array_slice (vec *v) >> >> >> + : m_base (v ? v->address () : nullptr), m_size (v ? v->length= () : 0) {} >> >> >> + >> >> > >> >> > I don?t quite understand why the generic ctor doesn?t cover the GC = case. It looks more like reference vs pointer? >> >> > >> >>=20 >> >> If you think that this should work: >> >>=20 >> >> vec *heh =3D cfun->local_decls; >> >> array_slice arr_slice (*heh); >> >>=20 >> >> then it does not: >> >>=20 >> >> /home/mjambor/gcc/mine/src/gcc/ipa-cp.cc:6693:36: error: no matchin= g function for call to ?array_slice::array_slice(vec&)? >> >> 6693 | array_slice arr_slice (*heh); >> >> | ^ >> >> In file included from /home/mjambor/gcc/mine/src/gcc/hash-table.h:2= 48, >> >> from /home/mjambor/gcc/mine/src/gcc/coretypes.h:48= 6, >> >> from /home/mjambor/gcc/mine/src/gcc/ipa-cp.cc:105: >> >> /home/mjambor/gcc/mine/src/gcc/vec.h:2264:3: note: candidate: ?temp= late array_slice::array_slice(const vec&) [with T = =3D tree_node*]? >> >> 2264 | array_slice (const vec &v) >> >> | ^~~~~~~~~~~ >> >> /home/mjambor/gcc/mine/src/gcc/vec.h:2264:3: note: template argum= ent deduction/substitution failed: >> >> /home/mjambor/gcc/mine/src/gcc/ipa-cp.cc:6693:36: note: mismatche= d types ?va_heap? and ?va_gc? >> >> 6693 | array_slice arr_slice (*heh); >> >> | ^ >> >>=20 >> >> [... I trimmed notes about all other candidates...] >> >>=20 >> >> Or did you mean something else? >> > >> > Hmm, so what if you change >> > >> > template >> > array_slice (const vec &v) >> > : m_base (v.address ()), m_size (v.length ()) {} >> > >> > to >> > >> > template >> > array_slice (const vec &v) >> > : m_base (v.address ()), m_size (v.length ()) {} >> > >> > instead? Thus allow any allocation / placement template arg? >> > >>=20 >> So being fully awake helps, the issue was of course in how I tested the >> code, the above works fine and I can adapt my code to use that. >>=20 >> However, is it really preferable? >>=20 >> We often use NULL as to mean zero-length vector, which my code handled >> gracefully: >>=20 >> + template >> + array_slice (const vec *v) >> + : m_base (v ? v->address () : nullptr), m_size (v ? v->length () : = 0) {} >>=20 >> whereas using the generic method will mean that users constructing the >> vector will have to special case it - and I bet most will end up using >> the above sequence and the constructor from explicit pointer and size in >> all constructors from gc vectors. >>=20 >> So, should I really change the patch and my code? > > Well, it's also inconsistent with a supposed use like > > vec *v =3D NULL; > auto slice =3D array_slice (v); > > no? So, if we want to provide a "safe" (as in, handle NULL pointer) > CTOR, don't we want to handle non-GC allocated vectors the same way? > Our safe_* functions usually do no work with normal non-GC vectors (which are not vl_embed), they do not accept them. I guess that is because that is not how we use normal vectors, we usually pass around vNULL to mean empty vector of that type. So I'd at least be consistent with our inconsistencies. But whatever, I can have both reference and pointer template constructors, I can resort to constructing them from v->address() and v->length() too. I do not care much, I guess I trust your sense of code esthetics more than mine, just please let me know what you prefer and I'll go with that. > Btw, we have > > template > array_slice (T (&array)[N]) : m_base (array), m_size (N) {} > > which would suggest handling NULL isn't desired(?) > That is not how I read for example: // True if the array is valid, false if it is an array like INVALID. bool is_valid () const { return m_base || m_size =3D=3D 0; } And IMHO it would be a very very strange limitation too. Martin