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 C93F83858D37 for ; Mon, 29 Aug 2022 11:26:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C93F83858D37 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 9D1D61F923; Mon, 29 Aug 2022 11:26:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1661772367; h=from:from:reply-to: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=0t2v0Y1AdMlwACD8TMjVXOSjAEnB3m5y1HkFMWDragc=; b=iGxtAsnauB2a88o9Uy1dcYNgn849rkDpDyWOlBmb5VIfNotuJBIuEKoVE/bPBAoK8HzkhJ Jv0/GcHP32Ar7/ZeMqLTDpshdLggiR4qBJpTaoxy+V3cQAkR5P5lY+BklR+gVcu2zxCZFZ TwTPDxiDBnpPMQ7975yqFZEFRMGLiuA= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1661772367; h=from:from:reply-to: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=0t2v0Y1AdMlwACD8TMjVXOSjAEnB3m5y1HkFMWDragc=; b=fX1GVu6wxzUQl9wpZcFQd08euStEQbCfLCzeNzdsEjYz9g08PtJG48pZDktunTcdLUZd2c tPno+yikS27DDDAg== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 73F312C141; Mon, 29 Aug 2022 11:26:07 +0000 (UTC) Date: Mon, 29 Aug 2022 11:26:07 +0000 (UTC) From: Richard Biener To: Martin Jambor cc: GCC Patches , Richard Sandiford Subject: Re: [PATCH 1/2] vec: Add array_slice constructors from non-const and gc vectors In-Reply-To: Message-ID: References: User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="-1609957120-1237240764-1661772367=:14286" X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_NONE,SPF_PASS,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: This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. ---1609957120-1237240764-1661772367=:14286 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT On Mon, 29 Aug 2022, Martin Jambor wrote: > Hi again, > > On Mon, Aug 29 2022, Richard Biener wrote: > > On Fri, 26 Aug 2022, Martin Jambor wrote: > > > >> Hi, > >> > >> On Fri, Aug 26 2022, Richard Biener wrote: > >> >> Am 26.08.2022 um 18:39 schrieb Martin Jambor : > >> >> > >> >> Hi, > >> >> > >> >> This patch adds constructors of array_slice that are required to > >> >> create them from non-const (heap or auto) vectors or from GC vectors. > >> >> > >> >> 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_slices. > >> >> 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? > >> > > >> > >> If you think that this should work: > >> > >> vec *heh = cfun->local_decls; > >> array_slice arr_slice (*heh); > >> > >> then it does not: > >> > >> /home/mjambor/gcc/mine/src/gcc/ipa-cp.cc:6693:36: error: no matching 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:248, > >> from /home/mjambor/gcc/mine/src/gcc/coretypes.h:486, > >> from /home/mjambor/gcc/mine/src/gcc/ipa-cp.cc:105: > >> /home/mjambor/gcc/mine/src/gcc/vec.h:2264:3: note: candidate: ?template array_slice::array_slice(const vec&) [with T = tree_node*]? > >> 2264 | array_slice (const vec &v) > >> | ^~~~~~~~~~~ > >> /home/mjambor/gcc/mine/src/gcc/vec.h:2264:3: note: template argument deduction/substitution failed: > >> /home/mjambor/gcc/mine/src/gcc/ipa-cp.cc:6693:36: note: mismatched types ?va_heap? and ?va_gc? > >> 6693 | array_slice arr_slice (*heh); > >> | ^ > >> > >> [... I trimmed notes about all other candidates...] > >> > >> 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? > > > > 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. > > However, is it really preferable? > > We often use NULL as to mean zero-length vector, which my code handled > gracefully: > > + template > + array_slice (const vec *v) > + : m_base (v ? v->address () : nullptr), m_size (v ? v->length () : 0) {} > > 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. > > So, should I really change the patch and my code? Well, it's also inconsistent with a supposed use like vec *v = NULL; auto slice = 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? Btw, we have template array_slice (T (&array)[N]) : m_base (array), m_size (N) {} which would suggest handling NULL isn't desired(?) Richard. ---1609957120-1237240764-1661772367=:14286-- 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 C93F83858D37 for ; Mon, 29 Aug 2022 11:26:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C93F83858D37 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 9D1D61F923; Mon, 29 Aug 2022 11:26:07 +0000 (UTC) Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 73F312C141; Mon, 29 Aug 2022 11:26:07 +0000 (UTC) Date: Mon, 29 Aug 2022 11:26:07 +0000 (UTC) From: Richard Biener To: Martin Jambor cc: GCC Patches , Richard Sandiford Subject: Re: [PATCH 1/2] vec: Add array_slice constructors from non-const and gc vectors In-Reply-To: Message-ID: References: User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, 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 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 29 Aug 2022 11:26:21 -0000 Message-ID: <20220829112607.1fCYRZOVi_W_gRLtSJVQB0FYdm5zHKRIAycrErL7n5I@z> On Mon, 29 Aug 2022, Martin Jambor wrote: > Hi again, > > On Mon, Aug 29 2022, Richard Biener wrote: > > On Fri, 26 Aug 2022, Martin Jambor wrote: > > > >> Hi, > >> > >> On Fri, Aug 26 2022, Richard Biener wrote: > >> >> Am 26.08.2022 um 18:39 schrieb Martin Jambor : > >> >> > >> >> Hi, > >> >> > >> >> This patch adds constructors of array_slice that are required to > >> >> create them from non-const (heap or auto) vectors or from GC vectors. > >> >> > >> >> 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_slices. > >> >> 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? > >> > > >> > >> If you think that this should work: > >> > >> vec *heh = cfun->local_decls; > >> array_slice arr_slice (*heh); > >> > >> then it does not: > >> > >> /home/mjambor/gcc/mine/src/gcc/ipa-cp.cc:6693:36: error: no matching 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:248, > >> from /home/mjambor/gcc/mine/src/gcc/coretypes.h:486, > >> from /home/mjambor/gcc/mine/src/gcc/ipa-cp.cc:105: > >> /home/mjambor/gcc/mine/src/gcc/vec.h:2264:3: note: candidate: ?template array_slice::array_slice(const vec&) [with T = tree_node*]? > >> 2264 | array_slice (const vec &v) > >> | ^~~~~~~~~~~ > >> /home/mjambor/gcc/mine/src/gcc/vec.h:2264:3: note: template argument deduction/substitution failed: > >> /home/mjambor/gcc/mine/src/gcc/ipa-cp.cc:6693:36: note: mismatched types ?va_heap? and ?va_gc? > >> 6693 | array_slice arr_slice (*heh); > >> | ^ > >> > >> [... I trimmed notes about all other candidates...] > >> > >> 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? > > > > 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. > > However, is it really preferable? > > We often use NULL as to mean zero-length vector, which my code handled > gracefully: > > + template > + array_slice (const vec *v) > + : m_base (v ? v->address () : nullptr), m_size (v ? v->length () : 0) {} > > 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. > > So, should I really change the patch and my code? Well, it's also inconsistent with a supposed use like vec *v = NULL; auto slice = 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? Btw, we have template array_slice (T (&array)[N]) : m_base (array), m_size (N) {} which would suggest handling NULL isn't desired(?) Richard.