From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 04C553857C72 for ; Tue, 30 Aug 2022 08:01:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 04C553857C72 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id C2C4E23A; Tue, 30 Aug 2022 01:01:12 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.62]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0F0D73F766; Tue, 30 Aug 2022 01:01:05 -0700 (PDT) From: Richard Sandiford To: Richard Biener Mail-Followup-To: Richard Biener ,Martin Jambor , GCC Patches , richard.sandiford@arm.com Cc: Martin Jambor , GCC Patches Subject: Re: [PATCH 1/2] vec: Add array_slice constructors from non-const and gc vectors References: Date: Tue, 30 Aug 2022 09:01:04 +0100 In-Reply-To: (Richard Biener's message of "Mon, 29 Aug 2022 12:34:10 +0000 (UTC)") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-50.1 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,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: Richard Biener writes: > On Mon, 29 Aug 2022, Martin Jambor wrote: > >> Hi, >>=20 >> 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 ve= ctors. >> >> >> >> >> >> >> >> The use of non-const array_slices is somewhat limited, as creat= ing one >> >> >> >> from const vec still leads to array_slice, >> >> >> >> so I eventually also only resorted to having read-only array_sl= ices. >> >> >> >> 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 refere= nce 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->len= gth () : 0) {} >> >> >> >> + >> >> >> >> + template >> >> >> >> + array_slice (vec *v) >> >> >> >> + : m_base (v ? v->address () : nullptr), m_size (v ? v->len= gth () : 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 matc= hing 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:1= 05: >> >> >> /home/mjambor/gcc/mine/src/gcc/vec.h:2264:3: note: candidate: ?t= emplate 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 ar= gument deduction/substitution failed: >> >> >> /home/mjambor/gcc/mine/src/gcc/ipa-cp.cc:6693:36: note: mismat= ched 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 t= he >> >> 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? >> > >>=20 >> 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. >>=20 >> 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. >>=20 >> > Btw, we have >> > >> > template >> > array_slice (T (&array)[N]) : m_base (array), m_size (N) {} >> > >> > which would suggest handling NULL isn't desired(?) >> > >>=20 >> That is not how I read for example: >>=20 >> // 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; } >>=20 >> 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. Yeah, the patch LGTM FWWIW. I agree it feels a bit weird to convert "pointer to vector of T" into "array-like of T" without a dereference, but avoiding it might be more convoluted than going with the flow. It doesn't look like it should introduce genuine ambiguity, since the T template parameter would always need to be specified explicitly. (But I don't think we should have a make_array_slice for vector pointers.) Thanks, Richard