From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa3.mentor.iphmx.com (esa3.mentor.iphmx.com [68.232.137.180]) by sourceware.org (Postfix) with ESMTPS id EE483385840D; Tue, 19 Dec 2023 15:42:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EE483385840D Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org EE483385840D Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=68.232.137.180 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1703000526; cv=none; b=Tr24M2/DRVx8YUZIYNgsJs2U+1/DUrXKWJ0u2YsjfRVNh/bQ2bSyneARpp3TE2hhR/hrzo6CVdsv5uBAALkyHFoOJlztdGWQMKLsDBUnog9amlrlkdDXMYwNuA5fa4ramwnMpiH5D2uCLFLukHOgCS9q3hKmOjj9ykCbbBbP57g= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1703000526; c=relaxed/simple; bh=MJSBNOmmkgTBTij5/obnoSi1wkZX8GB841X6+sJBtM8=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=jdvdFu1yFcHxBtg0iJ3ZzWboCjs/bC9rgxzkTxba96PdtGDCrxm8742f/Hprio6XUcpglXMpT+LBYGED7hk4LvRDGMIzCOfKbiEKcXafkixFdbzFpPiD0wDxu8xu0frn3d6UNjlXDCj6GiAkS/UWu3LuYMlEjyLID0LTZ/O3yI8= ARC-Authentication-Results: i=1; server2.sourceware.org X-CSE-ConnectionGUID: YABT6lh7TyK9cZ+nEyIU8A== X-CSE-MsgGUID: Xuu7iwNuTVKkvGIYrGI65A== X-IronPort-AV: E=Sophos;i="6.04,288,1695715200"; d="scan'208";a="25761970" Received: from orw-gwy-01-in.mentorg.com ([192.94.38.165]) by esa3.mentor.iphmx.com with ESMTP; 19 Dec 2023 07:42:01 -0800 IronPort-SDR: b4JPewSvTKlnHF4nXLjxifyaluLUiRuSMUAu6WRnpGe32EyqjPUs7LOt6mNFcTmfm89pWlGxZG uJo/Zy6u6P7wxGuAFWstr2tWvDSUK5fykFfSids1aExMOH5gV0ZTCAI2mqJTlPa0LC0zwNOi/x 6sh3HwAq/l8zViJGvAVQAjXvnJILGJjzCJpgoMAbjc5752GmUWt7F1TKaSoK0ytUCiwauf4PSd fUH/9xmXxq5tlFR/u7sEEtc5XKIoPZkzfObo64QVhXHqlm/bob09rKJpj8DPuKwkjCm5VciUBu qZI= Message-ID: <88ae5d38-f831-4538-94f6-c52bbc42c3e9@codesourcery.com> Date: Tue, 19 Dec 2023 16:41:54 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v7 5/5] OpenMP/OpenACC: Reorganise OMP map clause handling in gimplify.cc Content-Language: en-US To: Julian Brown , CC: References: <20231216132507.5991c79e@squid.athome> From: Tobias Burnus In-Reply-To: <20231216132507.5991c79e@squid.athome> 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=-11.3 required=5.0 tests=BAYES_00,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,KAM_SHORT,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 Julian, On 16.12.23 14:25, Julian Brown wrote: > OpenMP/OpenACC: Reorganise OMP map clause handling in gimplify.cc > > This patch has been separated out from the C++ "declare mapper" > support patch. It contains just the gimplify.cc rearrangement > work, mostly moving gimplification from gimplify_scan_omp_clauses > to gimplify_adjust_omp_clauses for map clauses. > > The motivation for doing this was that we don't know if we need to > instantiate mappers implicitly until the body of an offload region h= as > been scanned, i.e. in gimplify_adjust_omp_clauses, but we also need = the > un-gimplified form of clauses to sort by base-pointer dependencies a= fter > mapper instantiation has taken place. > > The patch also reimplements the "present" clause sorting code to avo= id > another sorting pass on mapping nodes. > > This version of the patch is based on the version posted for og13, a= nd > additionally incorporates a follow-on fix for DECL_VALUE_EXPR handli= ng > in gimplify_adjust_omp_clauses: > > "OpenMP/OpenACC: Reorganise OMP map clause handling in gimplify.cc" > https://gcc.gnu.org/pipermail/gcc-patches/2023-June/622223.html > > Parts of: > "OpenMP: OpenMP 5.2 semantics for pointers with unmapped target" > https://gcc.gnu.org/pipermail/gcc-patches/2023-June/623351.html I find the patch hard to digest - and I wouldn't be surprised if I missed something. However, I think the patch is OK - except for a few minor observations: > 2023-12-16 Julian Brown > > gcc/ > * gimplify.cc (omp_segregate_mapping_groups): Handle "presen= t" groups. > (gimplify_scan_omp_clauses): Use mapping group functionality= to > iterate through mapping nodes. Remove most gimplification o= f > OMP_CLAUSE_MAP nodes from here, but still populate ctx->vari= ables > splay tree. > (gimplify_adjust_omp_clauses): Move most gimplification of > OMP_CLAUSE_MAP nodes here. > > gcc/testsuite/ > * gfortran.dg/gomp/map-12.f90: Adjust scan output. > > libgomp/ > * testsuite/libgomp.fortran/target-enter-data-6.f90: Remove = XFAIL. > diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc > index a6bdceab45d..fa6ddd546f8 100644 > --- a/gcc/gimplify.cc > +++ b/gcc/gimplify.cc > @@ -10107,6 +10114,20 @@ omp_segregate_mapping_groups (omp_mapping_group = *inlist) > ard_tail =3D &w->next; > break; > > + case GOMP_MAP_PRESENT_ALLOC: > + *pa_tail =3D w; > + w->next =3D NULL; > + pa_tail =3D &w->next; > + break; > + > + case GOMP_MAP_PRESENT_FROM: > + case GOMP_MAP_PRESENT_TO: > + case GOMP_MAP_PRESENT_TOFROM: > + *ptf_tail =3D w; > + w->next =3D NULL; > + ptf_tail =3D &w->next; > + break; > + First, I note that GOMP_MAP_PRESENT_ALLOC and GOMP_MAP_PRESENT_{FROM,TO,TOFROM} are semantically identical: If the variable is not present, error termination will happen - otherwise, if present, no data movement will happen. Hence, they will be changed to GOMP_MAP_FORCE_PRESENT in gimplify_adjust_omp_clauses. That's also the reason that the old code handled all of them identical. However, besides a plain 'present', there is also 'always' + 'present'. Those are different as after a normal 'present' check (abort if not present), the data copying will happen: GOMP_MAP_ALWAYS_PRESENT_TO, GOMP_MAP_ALWAYS_PRESENT_FROM, GOMP_MAP_ALWAYS_PRESENT_TOFROM. (Note that: always + present + alloc =3D GOMP_MAP_PRESENT_ALLOC (w/o 'always') as already done in the FE.) Thus, all 'case' from your patch should go to a single group (possibly adding a comment about it). The question is what to do with the 'present,always' case. I think leaving them under 'default:' is fine, but I might have missed something. > default: > *tf_tail =3D w; > w->next =3D NULL; > @@ -10118,8 +10139,10 @@ omp_segregate_mapping_groups (omp_mapping_group = *inlist) * * * > @@ -11922,119 +11945,30 @@ gimplify_scan_omp_clauses (tree *list_p, gimpl= e_seq *pre_p, > break; > } > > - if (code =3D=3D OMP_TARGET > - || code =3D=3D OMP_TARGET_DATA > - || code =3D=3D OMP_TARGET_ENTER_DATA > - || code =3D=3D OMP_TARGET_EXIT_DATA) > - { > - vec *groups; > - groups =3D omp_gather_mapping_groups (list_p); > - if (groups) > - { > - hash_map *grpmap; > - grpmap =3D omp_index_mapping_groups (groups); > + vec *groups =3D omp_gather_mapping_groups (list_p); > + hash_map *grpmap =3D NUL= L; > + unsigned grpnum =3D 0; > + tree *grp_start_p =3D NULL, grp_end =3D NULL_TREE; ... > - else if (region_type & ORT_ACC) > - { I wonder whether you should not better call 'omp_gather_mapping_groups' only for the 'code =3D=3D OMP_TARGET...' and for ORT_ACC (or some subset of OACC *), given that this function is also called bygimplify_omp_parallel, gimplify_omp_task, gimplify_omp_for, ... This avoids some memory allocation and list_p walking, i.e. it is not too bad - but also not really needed for task, parallel, for, ... > @@ -14008,26 +13926,73 @@ gimplify_adjust_omp_clauses (gimple_seq *pre_p,= gimple_seq body, tree *list_p, > default: > break; > } > - if (code =3D=3D OMP_TARGET_EXIT_DATA > - && OMP_CLAUSE_MAP_KIND (c) =3D=3D GOMP_MAP_ALWAYS_POINTER) > + switch (code) > { > + case OMP_TARGET: > + break; > + case OACC_DATA: > + if (TREE_CODE (TREE_TYPE (decl)) !=3D ARRAY_TYPE) > + break; > + goto check_firstprivate; > + case OACC_ENTER_DATA: > + case OACC_EXIT_DATA: > + case OMP_TARGET_DATA: > + case OMP_TARGET_ENTER_DATA: > + case OMP_TARGET_EXIT_DATA: > + case OACC_HOST_DATA: > + check_firstprivate: > + if (OMP_CLAUSE_MAP_KIND (c) =3D=3D GOMP_MAP_FIRSTPRIVATE_POIN= TER I think it looks nicer if the OACC_HOST is before OMP_* such that all OACC_* are together. (In the old code, oacc_enter/exit was treated differently than OMP_* and OACC_HOST_DATA; your order is a leftover from that code movement/change.) * * * > + flags =3D GOVD_MAP | GOVD_EXPLICIT; > + if (OMP_CLAUSE_MAP_KIND (c) =3D=3D GOMP_MAP_ALWAYS_TO > + || OMP_CLAUSE_MAP_KIND (c) =3D=3D GOMP_MAP_ALWAYS_TOFROM) > + flags |=3D GOVD_MAP_ALWAYS_TO; I know that the code has only been moved, but I wonder whether that should also include GOMP_MAP_ALWAYS_PRESENT_{TO,TOFROM} as condition. 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