From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.126.133]) by sourceware.org (Postfix) with ESMTPS id 0BD7F3858D35; Sun, 7 Jan 2024 15:04:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0BD7F3858D35 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=net-b.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=net-b.de ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 0BD7F3858D35 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=212.227.126.133 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1704639889; cv=none; b=fHQTShfzseTZdGlEmDPcz1n+lOC9O4NT35pbvAqhk2IZp98TMNiIUtcam4fz+WD4gJY9boU7SnnwuA9Sxx1a2wgShGfGfCQGsq6OI99mqRK7qO5mGd1MHoiY7PbCc9OB6FC+gKX80QcPpi4LJ3FiSS6qzmtP0JDCdeQsrjzQbM4= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1704639889; c=relaxed/simple; bh=cvE9CJm3ZvNuspJSHOzRzTOJhTW4h+OdESLyk1e+RNw=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=XsBa69GcZFaTQ5Fmuxwdgv/zPv43Q4R3j/JkfnPh23xcWkRDenB451WInTBOJwUQd1K2KYC4lIPoW1keEf1yhxJeg5Ip1cegEADx5KE3QWP6GJ0MIBviVSpum+uvJZn5KZjH5twc7TLuIyuzmAf2k8owy3666qFMOsvEORsad64= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from [192.168.0.26] ([89.247.230.56]) by mrelayeu.kundenserver.de (mreue012 [213.165.67.97]) with ESMTPSA (Nemesis) id 1M7v18-1rHWtQ23Gs-0051LZ; Sun, 07 Jan 2024 16:04:38 +0100 Message-ID: <0f3ec635-a817-4fa8-b307-6f977cda8642@net-b.de> Date: Sun, 7 Jan 2024 16:04:37 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 1/8] OpenMP: lvalue parsing for map/to/from clauses (C++) Content-Language: en-US To: Julian Brown , Tobias Burnus Cc: gcc-patches@gcc.gnu.org, fortran@gcc.gnu.org, jakub@redhat.com References: <9a497499-31d7-4bb2-afdd-815966b49406@codesourcery.com> <20240105122326.1f89c4d6@squid.athome> From: Tobias Burnus In-Reply-To: <20240105122326.1f89c4d6@squid.athome> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Provags-ID: V03:K1:63d+ZKhbgSLvZTaihXrdh4JK8Yy341fk89P2jE0aS7vD95T+sP5 HXrwkf+ZuXJMD3e7qXPzzHssL7eF8nzy3yxlDMQYW/opotqsXQVTSMdG7ssS9OB0FjBJjpj 8k5ZbNUwGvnkSlldcJ6ljK249idHTVOGPeoWGjjuaw2VrQYauqwyqOQmrUeMEGd+pD/i50h LI3UIFnw+ByFy4bNRSLeA== UI-OutboundReport: notjunk:1;M01:P0:iVN1LHVmyVg=;7AaUcfXzOHFpRQDCER/M9jYuyG6 wTKMKRSrumTP8qEb+zVzxA+ibtbo7hqxd5u5E4N4vP++87S3l1WV2glzxUBqp2muPgQSH5z85 kYkqmS5MvZA6mYMSayN5GyUZ58vQwKJ9FcnBJN1hrxRlB1BYkJiv18Svqfw5DEHE8JhdyFOV4 7GmawyqrNRy78wfxckpRvLDRhTwTSCTTnfs7uNLTK915bb9wFhisP5VmwM8CTn+uY0FMtiJcn +ASqqvC/ZVIfEthNCP00wQ0JUI7aSU/Sk2ND1Reg1imTqikwUpMdTK2vn39HqIMPWwuWF85Nr KNWEbQckJnb6AjASOxxNi/FUXmdbCIxRJ6wuHNt4vaAwIfjBGM7vycQHBE2UO+kNDi+d3SPoS sghggRBG1Kct0n8OB1j8j6b5ahX1AAGs8MVc7iqmJBxj6a0m/aYOE0GE63mY2p9zGtibAkrl0 i9P/ukd5KVesJv5CYGR8E40z0bU3atCSxslTv5NjdHYnluqHFtrIhiRXDOzp1CrBIEsP4wP+4 Na08g5AnhYXc7mWr4KmDy80EirxPwat0y3N/6PtPTkQ3DnydzpFLe1oSzfMfMZqE6BN3nUZQC tzS2M7a26S5D3puoS+ORZsANFzOS81rSc4j99s+gMUSGqANwvmhcwwvzQTsguR9oktOiaudta iA1UIMUHK5UzbFE0dfwlBkRDDhhH0wRp17yLvro/E7ChK+I/g/34sDeoOTbbEkoF+J2+q7nEW NiqHQT3WkMVugiF6UcDU2Bh5ZNDERo3458FV2oiGmH5O99Vq5yC82r3lFEFIF+7R0MW2IDFEm wutztpISMeedfPZNXN3pMjMSdiU6FhmRregzNpHiqvfCCta9ca4tJelqBrNk6juscYX6R7QuF m3y3wMZiGNsysiA== X-Spam-Status: No, score=-0.4 required=5.0 tests=BAYES_00,KAM_DMARC_STATUS,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Am 05.01.24 um 13:23 schrieb Julian Brown: > On Wed, 20 Dec 2023 15:31:15 +0100 > Tobias Burnus wrote: > Here's a rebased/retested version which fixes those bits (I haven't > adjusted the libgomp.texi bit you noted yet, though). > > How does this look now? > --- a/gcc/gimplify.cc > +++ b/gcc/gimplify.cc > @@ -13499,7 +13499,11 @@ gimplify_adjust_omp_clauses_1 (splay_tree_node n, void *data) > if (TREE_CODE (dtype) == REFERENCE_TYPE) > dtype = TREE_TYPE (dtype); > /* FIRSTPRIVATE_POINTER doesn't work well if we have a > - multiply-indirected pointer. */ > + multiply-indirected pointer. If we have a reference to a pointer to > + a pointer, it's possible that this should really be > + GOMP_MAP_FIRSTPRIVATE_REFERENCE -- but that also doesn't work at the > + moment, so stick with this. (See testcase > + baseptrs-4.C:ref2ptrptr_offset_decl_member_slice). */ Looks as we should have a tracking PR about this; can you file one? * * * > + if (processing_template_decl) > + { > + if (type_dependent_expression_p (array_expr) > + || type_dependent_expression_p (index) > + || type_dependent_expression_p (length)) > + return build_min_nt_loc (loc, OMP_ARRAY_SECTION, array_expr, index, > + length); > + } I personally find it more readable if combined in a single 'if' condition. > + /* Turn *foo into foo[0:1]. */ > + decl = TREE_OPERAND (decl, 0); > + STRIP_NOPS (decl); > + > + /* If we have "*foo" and > + - it's an indirection of a reference, "unconvert" it, i.e. > + strip the indirection (to just "foo"). > + - it's an indirection of a pointer, turn it into > + "foo[0:1]". */ > + if (!ref_p) > + decl = grok_omp_array_section (loc, decl, integer_zero_node, > + integer_one_node); I would remove the first comment and remove the two succeeding lines below the second comment. > + /* This code rewrites a parsed expression containing various tree > + codes used to represent array accesses into a more uniform nest of > + OMP_ARRAY_SECTION nodes before it is processed by > + semantics.cc:handle_omp_array_sections_1. It might be more > + efficient to move this logic to that function instead, analysing > + the parsed expression directly rather than this preprocessed > + form. */ Or to do this transformation in handle_omp_array_sections to get still a unified result in the middle end. I see advantages of all three solutions. (Doing this in parse.cc (as currently done) feels a bit odd, though.) * * * > build_omp_array_section (location_t loc, tree array_expr, tree index, > + tree length) > +{ > + tree idxtype; > + > + /* If we know the integer bounds, create an index type with exact > + low/high (or zero/length) bounds. Otherwise, create an incomplete > + array type. (This mostly only affects diagnostics.) */ > + if (index != NULL_TREE > + && length != NULL_TREE > + && TREE_CODE (index) == INTEGER_CST > + && TREE_CODE (length) == INTEGER_CST) > + { > + tree low = fold_convert (sizetype, index); > + tree high = fold_convert (sizetype, length); > + high = size_binop (PLUS_EXPR, low, high); > + high = size_binop (MINUS_EXPR, high, size_one_node); > + idxtype = build_range_type (sizetype, low, high); > + } > + else if ((index == NULL_TREE || integer_zerop (index)) > + && length != NULL_TREE > + && TREE_CODE (length) == INTEGER_CST) > + idxtype = build_index_type (length); > + else > + idxtype = NULL_TREE; > + > + tree type = TREE_TYPE (array_expr); > + gcc_assert (type); > + type = non_reference (type); > + > + tree sectype, eltype = TREE_TYPE (type); > + > + /* It's not an array or pointer type. Just reuse the type of the > + original expression as the type of the array section (an error will be > + raised anyway, later). */ > + if (eltype == NULL_TREE) > + sectype = TREE_TYPE (array_expr); > + else > + sectype = build_array_type (eltype, idxtype); > + > + return build3_loc (loc, OMP_ARRAY_SECTION, sectype, array_expr, index, > + length); > +} I wonder whether it would be more readable if one moves all the 'idxtype' handling into the last 'else' branch. * * * LGTM - please file the PR and consider the readability items above. Thanks, Tobias