From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa2.mentor.iphmx.com (esa2.mentor.iphmx.com [68.232.141.98]) by sourceware.org (Postfix) with ESMTPS id B8FB93858D37 for ; Fri, 28 Apr 2023 17:26:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B8FB93858D37 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com X-IronPort-AV: E=Sophos;i="5.99,235,1677571200"; d="scan'208";a="4263696" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa2.mentor.iphmx.com with ESMTP; 28 Apr 2023 09:26:39 -0800 IronPort-SDR: KHpuVsFNS79J86p0bI6TBHdvIsjxMgjV39rBEcArRuddLAwythXwhzZlFImJQRI8b3m4jEWeh5 TAABUY1iGI0FEFavearJNlDEWxAphjBTAuznfpp7ATO4aW8RIjQr6Sj5GEXcRv+5ZQWHiZYnvB 326GAVv58a8Va+tc1hJH1YqQj/pYvz3d8Wjg8S/5OYlyn2pYG7FIxyznYYjHH3QeUBjV3hdx26 bepOZSluhb0BBpjlOgtSgQOjullbKHBrbQq/w1ztJVmwQKYw1BGsRzemmSPPbElaSrcnULKIAO MEM= Message-ID: <1882e7aa-6e79-25ff-4ee4-8152106a5935@codesourcery.com> Date: Fri, 28 Apr 2023 19:26:30 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.1 Subject: Re: [PATCHv2] openmp: Add support for 'present' modifier Content-Language: en-US To: Kwok Cheung Yeung , gcc-patches , Jakub Jelinek References: <6eb5d0dd-da2a-6d8e-eaa2-d14bf708cf36@codesourcery.com> From: Tobias Burnus In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: quoted-printable X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-12.mgc.mentorg.com (139.181.222.12) To svr-ies-mbx-12.mgc.mentorg.com (139.181.222.12) X-Spam-Status: No, score=-6.0 required=5.0 tests=BAYES_00,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,NICE_REPLY_A,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,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: Hi Kwok, On 17.02.23 12:45, Kwok Cheung Yeung wrote: > This is a revised version of the patch for the 'present' modifier for > OpenMP. Compared to the first version, three improvements have been made: > > - A bug which caused bootstrapping with a '-m32' multilib on x86-64 to > fail due to pointer size issues has been fixed. > - The Fortran parse tree dump now shows clauses with 'present' applied. > - The reordering of OpenMP clauses has been moved to > gimplify_scan_omp_clauses, where the other clause reordering rules are > applied. thanks for the patch; I have a bunch of smaller review comments, requiring = small code changes or more tedious but still simple changes. Namely: In the ChangeLog: > (c_parser_omp_target_data): Allow map clauses with 'present' > modifiers. > (c_parser_omp_target_enter_data): Likewise. > (c_parser_omp_target_exit_data): Likewise. > (c_parser_omp_target): Likewise. Those be combined; a separate entry is only required per file not per function name. > + if (kind =3D=3D OMP_CLAUSE_FROM || kind =3D=3D OMP_CLAUSE_T= O) > + OMP_CLAUSE_SET_MOTION_MODIFIER (u, OMP_CLAUSE_MOTION_NONE= ); This should not be needed as 'build_omp_clause' memset '\0' the data and OMP_CLAUSE_MOTION_NONE =3D=3D 0 (as it should). However, as you really only have two values, denoting that the modifier has been specified or not, you should really use an available existing flag. Fo= r instance, other code uses base.deprecated_flag =E2=80=93 which could also be used her= e. Macro wise, this would then permit to use: OMP_CLAUSE_MOTION_PRESENT (node) =3D 1; or OMP_CLAUSE_TO_PRESENT (node) =3D 1; OMP_CLAUSE_FROM_PRESENT (node) =3D 1; and 'if (OMP_... (node))' which is shorter and is IMHO to be also more read= able. * * * I think c_parser_omp_var_list_parens / cp_parser_omp_var_list / gfc_match_o= mp_variable_list should not be modified. For C/C++, you just could do the '(' + {'present', ':' } parsing before the= call to c_parser_omp_variable_list / cp_parser_omp_var_list_no_open and then loop over 'list' after the call - and similarly for Fortran. And besides not cluttering a generic function, we will also soon add suppor= t for 'mapper' (=E2=86=92 Julian's patch set adds generic mapper support) and 'it= erator' is also missing. And we really do not want those in the generic function! > + kind =3D always_present_modifier ? GOMP_MAP_ALWAYS_PRESENT_FROM > + : present_modifier ? GOMP_MAP_PRESENT_FROM > + : always_modifier ? GOMP_MAP_ALWAYS_FROM > + : GOMP_MAP_FROM; Can you wrap the RHS in parenthesis, i.e. 'kind =3D (' ... ');' to aid some editors in terms of indenting. (I assume 'emacs' is that editor, which I do= n't use.) > + tkind > + =3D OMP_CLAUSE_MOTION_MODIFIER (c) =3D=3D OMP_CLAUSE_MO= TION_PRESENT > + ? GOMP_MAP_PRESENT_TO : GOMP_MAP_TO; Likewise. * * * > @@ -1358,6 +1371,7 @@ typedef struct gfc_omp_namelist > ENUM_BITFIELD (gfc_omp_linear_op) op:4; > bool old_modifier; > } linear; > + gfc_omp_motion_modifier motion_modifier; > struct gfc_common_head *common; > bool lastprivate_conditional; > } u; I think a 'bool present;' would do here. Can you additionally move the pointers first and then the bitfields/enums later? That way, less space is wasted by padding and we might even save space despite adding another variable. > @@ -2893,20 +2912,38 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, cons= t omp_mask mask, > if (close_modifier++ =3D=3D 1) > second_close_locus =3D current_locus; > } > + else if (gfc_match ("present ") =3D=3D MATCH_YES) > + { > + if (present_modifier++ =3D=3D 1) > + second_close_locus =3D current_locus; > + } > ... This code is broken in terms of error diagnostic. You need to handle this differently to get diagostic for: 'map(present, present : var)' Can you fix the code + add a testcase (e.g. by augmenting the 'always' test= case files testsuite/gfortran.dg/gomp/map-{7,8}.f90). > + gomp_fatal ("present clause: !omp_target_is_present " I personally find the error a bit unclear. How about something more explici= t like: 'present clause: not present on the device' - or something like that? > +/* { dg-do run { target offload_target_any } } */ This needs to be '{ target offload_device }' - i.e. the default device need= s to be an offload device (!=3D the initial device). (Side note: We may need to consider whether offload_device_nonshared_as mig= ht make sense, but the current omp_target_is_present and omp_target_(dis)associate_ptr implies= that we will still go though this route with USM for explicitly mapped variables (but do not do any actual mapping in that case). But that we can handle, once the USM code gets merged and we get FAILs.) Tobias ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra=C3=9Fe 201= , 80634 M=C3=BCnchen; Gesellschaft mit beschr=C3=A4nkter Haftung; Gesch=C3= =A4ftsf=C3=BChrer: Thomas Heurung, Frank Th=C3=BCrauf; Sitz der Gesellschaf= t: M=C3=BCnchen; Registergericht M=C3=BCnchen, HRB 106955