From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 373DA388EC34 for ; Tue, 13 Oct 2020 13:01:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 373DA388EC34 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-61-EqXOIOWxMSqetq-I_SIeWQ-1; Tue, 13 Oct 2020 09:01:44 -0400 X-MC-Unique: EqXOIOWxMSqetq-I_SIeWQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 638B18030C0; Tue, 13 Oct 2020 13:01:43 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-112-37.ams2.redhat.com [10.36.112.37]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6A39D55764; Tue, 13 Oct 2020 13:01:41 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.15.2/8.15.2) with ESMTP id 09DD1dwa010175; Tue, 13 Oct 2020 15:01:40 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id 09DD1cjp010172; Tue, 13 Oct 2020 15:01:38 +0200 Date: Tue, 13 Oct 2020 15:01:38 +0200 From: Jakub Jelinek To: Chung-Lin Tang Cc: gcc-patches , Tobias Burnus , Catherine Moore , Thomas Schwinge Subject: Re: [PATCH, 1/3, OpenMP] Target mapping changes for OpenMP 5.0, front-end parts Message-ID: <20201013130138.GZ2176@tucnak> Reply-To: Jakub Jelinek References: <639a56ef-eeed-eb38-8a19-f5cf8d082973@codesourcery.com> MIME-Version: 1.0 In-Reply-To: <639a56ef-eeed-eb38-8a19-f5cf8d082973@codesourcery.com> User-Agent: Mutt/1.11.3 (2019-02-01) X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-6.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham 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, 13 Oct 2020 13:01:59 -0000 On Tue, Sep 01, 2020 at 09:16:23PM +0800, Chung-Lin Tang wrote: > this patch set implements parts of the target mapping changes introduced > in OpenMP 5.0, mainly the attachment requirements for pointer-based > list items, and the clause ordering. > > The first patch here are the C/C++ front-end changes. > > The entire set of changes has been tested for without regressions for > the compiler and libgomp. Hope this is ready to commit to master. Sorry for the delay in patch review and thanks for the standard citations, that really helps. > gcc/c-family/ > * c-common.h (c_omp_adjust_clauses): New declaration. > * c-omp.c (c_omp_adjust_clauses): New function. Besides the naming, I wonder why is it done in a separate function and so early, can't what the function does be done either in {,c_}finish_omp_clauses (provided we'd pass separate ORT_OMP vs. ORT_OMP_TARGET to it to determine if it is target region vs. anything else), or perhaps even better during gimplification (gimplify_scan_omp_clauses)? > gcc/c/ > * c-parser.c (c_parser_omp_target_data): Add use of > new c_omp_adjust_clauses function. Add GOMP_MAP_ATTACH_DETACH as > handled map clause kind. > (c_parser_omp_target_enter_data): Likewise. > (c_parser_omp_target_exit_data): Likewise. > (c_parser_omp_target): Likewise. > * c-typeck.c (handle_omp_array_sections): Adjust COMPONENT_REF case to > use GOMP_MAP_ATTACH_DETACH map kind for C_ORT_OMP region type. > (c_finish_omp_clauses): Adjust bitmap checks to allow struct decl and > same struct field access to co-exist on OpenMP construct. > > gcc/cp/ > * parser.c (cp_parser_omp_target_data): Add use of > new c_omp_adjust_clauses function. Add GOMP_MAP_ATTACH_DETACH as > handled map clause kind. > (cp_parser_omp_target_enter_data): Likewise. > (cp_parser_omp_target_exit_data): Likewise. > (cp_parser_omp_target): Likewise. > * semantics.c (handle_omp_array_sections): Adjust COMPONENT_REF case to > use GOMP_MAP_ATTACH_DETACH map kind for C_ORT_OMP region type. Fix > interaction between reference case and attach/detach. > (finish_omp_clauses): Adjust bitmap checks to allow struct decl and > same struct field access to co-exist on OpenMP construct. The changelog has some 8 space indented lines. > + for (tree c = clauses; c; c = OMP_CLAUSE_CHAIN (c)) > + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP > + && OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_FIRSTPRIVATE_POINTER > + && TREE_CODE (TREE_TYPE (OMP_CLAUSE_DECL (c))) != ARRAY_TYPE) > + { > + tree ptr = OMP_CLAUSE_DECL (c); > + bool ptr_mapped = false; > + if (is_target) > + { > + for (tree m = clauses; m; m = OMP_CLAUSE_CHAIN (m)) Isn't this O(n^2) in number of clauses? I mean, e.g. for the equality comparisons (but see below) it could be dealt with e.g. using some bitmap with DECL_UIDs. > + if (OMP_CLAUSE_CODE (m) == OMP_CLAUSE_MAP > + && OMP_CLAUSE_DECL (m) == ptr Does it really need to be equality? I mean it will be for map(tofrom:ptr) map(tofrom:ptr[:32]) but what about e.g. map(tofrom:structx) map(tofrom:structx.ptr[:32]) ? It is true that likely we don't parse this yet though. > + && (OMP_CLAUSE_MAP_KIND (m) == GOMP_MAP_ALLOC > + || OMP_CLAUSE_MAP_KIND (m) == GOMP_MAP_TO > + || OMP_CLAUSE_MAP_KIND (m) == GOMP_MAP_FROM > + || OMP_CLAUSE_MAP_KIND (m) == GOMP_MAP_TOFROM)) What about the always modified mapping kinds? > + { > + ptr_mapped = true; > + break; > + } > + > + if (!ptr_mapped > + && DECL_P (ptr) > + && is_global_var (ptr) > + && lookup_attribute ("omp declare target", > + DECL_ATTRIBUTES (ptr))) > + ptr_mapped = true; > + } > + > + /* If the pointer variable was mapped, or if this is not an offloaded > + target region, adjust the map kind to attach/detach. */ > + if (ptr_mapped || !is_target) > + { > + OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_ATTACH_DETACH); > + c_common_mark_addressable_vec (ptr); Though perhaps this is argument why it needs to be done in the FEs and not during gimplification, because it is hard to mark something addressable at that point. > --- a/gcc/c/c-typeck.c > +++ b/gcc/c/c-typeck.c > @@ -13580,16 +13580,17 @@ handle_omp_array_sections (tree c, enum c_omp_region_type ort) > break; > } > tree c2 = build_omp_clause (OMP_CLAUSE_LOCATION (c), OMP_CLAUSE_MAP); > if (ort != C_ORT_OMP && ort != C_ORT_ACC) > OMP_CLAUSE_SET_MAP_KIND (c2, GOMP_MAP_POINTER); > else if (TREE_CODE (t) == COMPONENT_REF) > { > - gomp_map_kind k = (ort == C_ORT_ACC) ? GOMP_MAP_ATTACH_DETACH > - : GOMP_MAP_ALWAYS_POINTER; > + gomp_map_kind k > + = ((ort == C_ORT_ACC || ort == C_ORT_OMP) > + ? GOMP_MAP_ATTACH_DETACH : GOMP_MAP_ALWAYS_POINTER); So what kind of C_ORT_* would be left after this change? C_ORT_*DECLARE_SIMD shouldn't have any kind of array sections in it. So maybe just > OMP_CLAUSE_SET_MAP_KIND (c2, k); OMP_CLAUSE_SET_MAP_KIND (c2, GOMP_MAP_ALWAYS_POINTER); ? > if (VAR_P (t) || TREE_CODE (t) == PARM_DECL) > { > - if (bitmap_bit_p (&map_field_head, DECL_UID (t))) > + if (bitmap_bit_p (&map_field_head, DECL_UID (t)) > + || bitmap_bit_p (&map_head, DECL_UID (t))) > break; Shall this change apply to OpenACC too? > } > } > if (!VAR_P (t) && TREE_CODE (t) != PARM_DECL) > { > error_at (OMP_CLAUSE_LOCATION (c), > "%qE is not a variable in %qs clause", t, > @@ -14751,29 +14753,36 @@ c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort) > error_at (OMP_CLAUSE_LOCATION (c), > "%qD appears both in data and map clauses", t); > remove = true; > } > else > bitmap_set_bit (&generic_head, DECL_UID (t)); > } > - else if (bitmap_bit_p (&map_head, DECL_UID (t))) > + else if (bitmap_bit_p (&map_head, DECL_UID (t)) > + && !bitmap_bit_p (&map_field_head, DECL_UID (t))) Ditto. Otherwise, what shall this diagnose now that the restriction that the same list item may not appear in multiple clauses is gone. > { > if (OMP_CLAUSE_CODE (c) != OMP_CLAUSE_MAP) > error_at (OMP_CLAUSE_LOCATION (c), > "%qD appears more than once in motion clauses", t); > else if (ort == C_ORT_ACC) > error_at (OMP_CLAUSE_LOCATION (c), > "%qD appears more than once in data clauses", t); > else > error_at (OMP_CLAUSE_LOCATION (c), > "%qD appears more than once in map clauses", t); > remove = true; So what is this supposed to diagnose now that the restriction that C++ ditto. Jakub