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 7CB3B3858407; Thu, 21 Dec 2023 08:51:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7CB3B3858407 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 7CB3B3858407 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=68.232.141.98 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1703148708; cv=none; b=GrQrWcvNDV3q/7YZBJb/JqTdndAs9Ztwh1+PquEacxCVKlqoAUMsr1t4M/YNOGXuA3KHiDwer7bdmvV1igkdqK99d6hsMPt9tS6hsP/PcVQwceFLzoQzIt0IbfonqCrgDrtRsbApHtfhdCzdWY6Qod9XiclVRrZRze6F6BA8+V0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1703148708; c=relaxed/simple; bh=bP2X/WsftjcwRECIFAqYzOZv7jBwAOr+FLJ8lozndmo=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=V0uXikjgf8nyxbsnCTptAQtSBQCYNgUAhqcAoHpCPLTuAHq2pw+6JFU+aCmqN/dYW/Gr+GniTW7o21B5NbJI2K+4XC2S5FiIFPWJBHcGD3nJhl9oDjQsL7GxR14rauixtg2kQDzrKobgnNIYKq8tkRSxCDi65vNchDvRs+EZ5TY= ARC-Authentication-Results: i=1; server2.sourceware.org X-CSE-ConnectionGUID: ZtRJWtE0TFOsqOh8eeyH5Q== X-CSE-MsgGUID: WXSkd73uTyC1JzV4tbp/oA== X-IronPort-AV: E=Sophos;i="6.04,293,1695715200"; d="scan'208";a="28984278" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa2.mentor.iphmx.com with ESMTP; 21 Dec 2023 00:51:45 -0800 IronPort-SDR: ChhNQb3BuQYEbdMXLiZF9Xi60t/HxN01IshtGY4keA8qW6xlyA3nIHY1KYHfEzyiC5n0qOAb8n +haT6iuRXLwiVPJjFsiV3tDHrHorLaXYdu8+6TuU66esCg1qvXF9XmvPtTRdVf/wx5P2Phi5vV yy8gKZX+zMbM2ptYYzFJD4M1KROcFs8/28r0iQISWwOWcwjq8/Qa1aqTv33f7sA4vhVtQAnOaY TngN+tCBh2MS1m/avRYAEI7lkikrDMfWgB6YxSqOXkYq++6tLnBnO5C8k2SpbKjneG9cKYaTGc Zr4= Message-ID: <3ed36e08-e0f1-47ee-84cc-0c3d8365d630@codesourcery.com> Date: Thu, 21 Dec 2023 09:51:39 +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> <88ae5d38-f831-4538-94f6-c52bbc42c3e9@codesourcery.com> <20231220212928.536df276@squid.athome> From: Tobias Burnus In-Reply-To: <20231220212928.536df276@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-14.mgc.mentorg.com (139.181.222.14) To svr-ies-mbx-12.mgc.mentorg.com (139.181.222.12) X-Spam-Status: No, score=-5.3 required=5.0 tests=BAYES_00,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,SPF_HELO_PASS,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: Hi Julian, On 20.12.23 22:29, Julian Brown wrote: > Thanks for review! Here's a new version of the patch which hopefully > addresses this round of comments. Thanks for the patch. LGTM now. Tobias > On Tue, 19 Dec 2023 16:41:54 +0100 > Tobias Burnus wrote: > >> On 16.12.23 14:25, Julian Brown wrote: >>> --- 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. > I've made this change (i.e.: grouping all "GOMP_MAP_PRESENT_*" nodes > together), and in fact that restores the dump output for the > gfortran.dg/gomp/map-12.f90 that needed to be adjusted for the previous > version of the patch (so that hunk has now disappeared). > >>> 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, >>> gimple_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 >>> NULL; >>> + 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, ... > I've made that change -- OpenACC uses OMP_CLAUSE_MAP in quite a wide > range of directives, but the new version of the patch lists them > individually anyway, rather than using a catch-all for ORT_ACC regions. > That seems OK, I think. > >>> @@ -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_POINTER >> 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.) > I've fixed this bit -- which actually doesn't need the goto any more > either, so that's now a fallthrough instead. > >>> + 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. > I've added it (caveat: without any tests). > > Re-tested with offloading to NVPTX. OK now? ----------------- 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