From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id E83D63857425 for ; Mon, 29 Aug 2022 12:34:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E83D63857425 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-out1.suse.de (Postfix) with ESMTP id 4869E229CB; Mon, 29 Aug 2022 12:34:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1661776450; 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=HrRQqlQZvsZL6QWLXfFOiMQOjOoM/v+ZbIlQGj7PBOw=; b=GQIOYMeF+vIjzTegkkN3OetoseO5Xa3ypr3LuKPV41SoM8Iyq3Edv8IGNdAh/EQaEet4a+ /yXk2GwWxFXtb+EnroM7J5OIRiDDa8QnvqsOMlv1LO+9UOvYdKIES35ToBJxOgcnw6/7rZ Z3OMfmhHlGMye53bFZqiuytHpGx7FK4= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1661776450; 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=HrRQqlQZvsZL6QWLXfFOiMQOjOoM/v+ZbIlQGj7PBOw=; b=80NqaFhqY3EE1lzHLgTUXRETfalLTMhtSUmQnjAHnLqHTulKVW3a8uRfcwfnvtvrx7m3hB uIQK4l/YIJWQCJCQ== 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 2562F2C141; Mon, 29 Aug 2022 12:34:10 +0000 (UTC) Date: Mon, 29 Aug 2022 12:34:10 +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-1936098924-1661776450=: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-1936098924-1661776450=:14286 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT On Mon, 29 Aug 2022, Martin Jambor wrote: > Hi, > > On Mon, Aug 29 2022, Richard Biener wrote: > > 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? > > > > 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 == 0; } > > And IMHO it would be a very very strange limitation too. I see. That said, the high number of CTORs does look a bit odd, but I'm fine with them if Richard is. Thanks and sorry for throwing in wrenches, Richard. ---1609957120-1936098924-1661776450=:14286-- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id E83D63857425 for ; Mon, 29 Aug 2022 12:34:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E83D63857425 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 4869E229CB; Mon, 29 Aug 2022 12:34:10 +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 2562F2C141; Mon, 29 Aug 2022 12:34:10 +0000 (UTC) Date: Mon, 29 Aug 2022 12:34:10 +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 12:34:18 -0000 Message-ID: <20220829123410.TJF-PY4mEQ90v4Jpfgkxzbd4pWlIUYn42dAP1AaohqE@z> On Mon, 29 Aug 2022, Martin Jambor wrote: > Hi, > > On Mon, Aug 29 2022, Richard Biener wrote: > > 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? > > > > 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 == 0; } > > And IMHO it would be a very very strange limitation too. I see. That said, the high number of CTORs does look a bit odd, but I'm fine with them if Richard is. Thanks and sorry for throwing in wrenches, Richard.