From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa4.mentor.iphmx.com (esa4.mentor.iphmx.com [68.232.137.252]) by sourceware.org (Postfix) with ESMTPS id 0D1A03844023; Tue, 18 May 2021 11:13:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0D1A03844023 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=ChungLin_Tang@mentor.com IronPort-SDR: klxTqi3gDgUd6gYECZH3rjcAqiWvrHk6dQATCpdC79OlPMx7ApjAI5YDvFtBcW8h7IIL3wVZqG VDrPS6kjLKz3Pi6nALitTBvza64UvUcbP9l+Cait60qBhanK9PfM1ys3k/8MsjQTw8CIRUGrTT NxGCH8sVU1v67p2W2GZ/A+mXoY68kX1BJGs4xGsJTstNSd0bahF0BiwhaKDVKE636QfzSvrn28 WMev7/ZKjzlghs2vvd2LKt6em5kRt27Fo3+jduXY9zHeVQcVCAFev5lQpJprNoBLO9qmrZD/9C 1QM= X-IronPort-AV: E=Sophos;i="5.82,310,1613462400"; d="scan'208";a="61480470" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa4.mentor.iphmx.com with ESMTP; 18 May 2021 03:13:22 -0800 IronPort-SDR: bvrd1b65jPLfiBcymRdd7Y/9mPPNJwvsDBFYSGDnf6IneuBrC+puK5WWWRnkuMUMU2A8KLd8Js E8gVfx3wxfy1aR0kADk05YbElRDURAmHXoxVcAYfPAJmIhYgz/OZdSh5uGCDIt5w95n2rjYInw mkZ4vRbXGi8DnlJo+zH5rFXS4MAqdIDyMX/UngiIokVhlcGvkSFwH/Q76CUtTuc6qpMG25MQWd QVMvBAuUbRQRN6FWZmTcZXmOEnSjCgoem+LqQtkgChkWJsnPCID0WdIt0Q8b80S2DCiug7DApq d0I= Subject: Re: [PATCH 7/7] [og10] WIP GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION changes To: Julian Brown CC: , Jakub Jelinek , Thomas Schwinge , , Tobias Burnus References: <04b90981e94acbabde004ee0992d404931e4fabf.1620721888.git.julian@codesourcery.com> <20210517152606.32920dcb@squid.athome> From: Chung-Lin Tang Message-ID: <359f2741-a3e6-7f7d-cd8f-c258e8936a2d@codesourcery.com> Date: Tue, 18 May 2021 19:13:12 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20210517152606.32920dcb@squid.athome> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-ClientProxiedBy: svr-orw-mbx-08.mgc.mentorg.com (147.34.90.208) To svr-orw-mbx-02.mgc.mentorg.com (147.34.90.202) X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 May 2021 11:13:34 -0000 On 2021/5/17 10:26 PM, Julian Brown wrote: > OK, understood. But, I'm a bit concerned that we're ignoring some > "hidden rules" with regards to OMP pointer clause ordering/grouping that > certain code (at least the bit that creates GOMP_MAP_STRUCT node > groups, and parts of omp-low.c) relies on. I believe those rules are as > follows: > > - an array slice is mapped using two or three pointers -- two for a > normal (non-reference) base pointer, and three if we have a > reference to a pointer (i.e. in C++) or an array descriptor (i.e. in > Fortran). So we can have e.g. > > GOMP_MAP_TO > GOMP_MAP_ALWAYS_POINTER > > GOMP_MAP_TO > GOMP_MAP_.*_POINTER > GOMP_MAP_ALWAYS_POINTER > > GOMP_MAP_TO > GOMP_MAP_TO_PSET > GOMP_MAP_ALWAYS_POINTER > > - for OpenACC, we extend this to allow (up to and including > gimplify.c) the GOMP_MAP_ATTACH_DETACH mapping. So we can have (for > component refs): > > GOMP_MAP_TO > GOMP_MAP_ATTACH_DETACH > > GOMP_MAP_TO > GOMP_MAP_TO_PSET > GOMP_MAP_ATTACH_DETACH > > GOMP_MAP_TO > GOMP_MAP_.*_POINTER > GOMP_MAP_ATTACH_DETACH > > For the scanning in insert_struct_comp_map (as it is at present) to > work right, these groups must stay intact. I think the current > behaviour of omp_target_reorder_clauses on the og10 branch can break > those groups apart though! Originally this sorting was intended to enforce OpenMP 5.0 map ordering rules, although I did add some ATTACH_DETACH ordering code in the latest round of patching. May not be the best practice. > (The "prev_list_p" stuff in the loop in question in gimplify.c just > keeps track of the first node in these groups.) Such a brittle way of doing this; even the variable name is not that obvious in what it intends to do. > For OpenACC, the GOMP_MAP_ATTACH_DETACH code does*not* depend on the > previous clause when lowering in omp-low.c. But GOMP_MAP_ALWAYS_POINTER > does! And in one case ("update" directive), GOMP_MAP_ATTACH_DETACH is > rewritten to GOMP_MAP_ALWAYS_POINTER, so for that case at least, the > dependency on the preceding mapping node must stay intact. Yes, I think there are some weird conventions here, stemming from the front-ends. I would think that _ALWAYS_POINTER should exist at a similar level like _ATTACH_DETACH, both a pointer operation, just different details in runtime behavior, though its intended purpose for C++ references seem to skew some things here and there. > OpenACC also allows "bare" GOMP_MAP_ATTACH and GOMP_MAP_DETACH nodes > (corresponding to the "attach" and "detach" clauses). Those are handled > a bit differently to GOMP_MAP_ATTACH_DETACH in gimplify.c -- but > GOMP_MAP_ATTACH_Z_L_A_S doesn't quite behave like that either, I don't > think? IIRC, GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION was handled that way (just a single line in gimplify.c) due to idiosyncrasies with the surrounding generated maps from the C++ front-end (which ATM is the only user of this map-kind). So yeah, inside the compiler, its not entirely the same as GOMP_MAP_ATTACH, but it is intended to live through for the runtime to see. > Anyway: I've not entirely understood what omp_target_reorder_clauses is > doing, but I think it may need to try harder to keep the groups > mentioned above together. What do you think? As you know, attach operations don't really need to be glued to the prior operations, it just has to be ordered after mapping of the pointer and the pointed. There's already some book-keeping to move clauses together, but as you say, it might need more. Overall, I think this re-organizing of the struct-group creation is a good thing, but actually as you probably also observed, this insistence of "in-flight" tree chain manipulation is just hard to work with and modify. Maybe instead of directly working on clause expression chains at this point, we should be stashing all this information into a single clause tree node, e.g. starting from the front-end, we can set 'OMP_CLAUSE_MAP_POINTER_KIND(c) = ALWAYS/ATTACH_DETACH/FIRSTPRIVATE/etc.', (instead of actually creating new, must-follow-in-order maps that's causing all these conventions). For struct-groups, during the start of gimplify_scan_omp_clauses(), we could work with map clause tree nodes with OMP_CLAUSE_MAP_STRUCT_LIST(c), which contains the entire TREE_LIST or VEC of elements. Then later, after scanning is complete, expand the list into the current form. Ordering is only created at this stage. Just an idea, not sure if it will help understandability in general, but it should definitely help to simplify when we're reordering due to other rules. Chung-Lin