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 [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 18ADD382BD37 for ; Tue, 24 May 2022 13:32:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 18ADD382BD37 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-67-NdURs0FuPVSih7pmsQVbeQ-1; Tue, 24 May 2022 09:32:51 -0400 X-MC-Unique: NdURs0FuPVSih7pmsQVbeQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 421AE8038E3; Tue, 24 May 2022 13:32:51 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.106]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D95097AD8; Tue, 24 May 2022 13:32:50 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 24ODWkxp2107043 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Tue, 24 May 2022 15:32:47 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 24ODWjoV2107042; Tue, 24 May 2022 15:32:45 +0200 Date: Tue, 24 May 2022 15:32:45 +0200 From: Jakub Jelinek To: Julian Brown Cc: gcc-patches@gcc.gnu.org, Thomas Schwinge , Tobias Burnus , Fortran List Subject: Re: [PATCH v2 04/11] OpenMP/OpenACC: Add inspector class to unify mapped address analysis Message-ID: Reply-To: Jakub Jelinek References: <306b9d1c02a6c2bdacd91afbc4edbf687437d336.1647619144.git.julian@codesourcery.com> MIME-Version: 1.0 In-Reply-To: <306b9d1c02a6c2bdacd91afbc4edbf687437d336.1647619144.git.julian@codesourcery.com> X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 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=-4.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SHORT, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, 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 X-BeenThere: fortran@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Fortran mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 24 May 2022 13:32:56 -0000 On Fri, Mar 18, 2022 at 09:24:54AM -0700, Julian Brown wrote: > 2022-03-17 Julian Brown > > gcc/c-family/ > * c-common.h (c_omp_address_inspector): New class. > * c-omp.c (c_omp_address_inspector::get_deref_origin, > c_omp_address_inspector::component_access_p, > c_omp_address_inspector::check_clause, > c_omp_address_inspector::get_root_term, Spaces instead of tabs. > c_omp_address_inspector::map_supported_p, > c_omp_address_inspector::mappable_type, > c_omp_address_inspector::get_origin, > c_omp_address_inspector::peel_components, > c_omp_address_inspector::maybe_peel_ref, > c_omp_address_inspector::maybe_zero_length_array_section, > c_omp_address_inspector::get_base_pointer, > c_omp_address_inspector::get_base_pointer_tgt, > c_omp_address_inspector::get_attachment_point): New methods. > --- a/gcc/c-family/c-common.h > +++ b/gcc/c-family/c-common.h > @@ -1253,6 +1253,61 @@ extern void c_omp_mark_declare_variant (location_t, tree, tree); > extern const char *c_omp_map_clause_name (tree, bool); > extern void c_omp_adjust_map_clauses (tree, bool); > > +class c_omp_address_inspector > +{ > + location_t loc; > + tree root_term; > + bool indirections; > + int map_supported; > + > +protected: > + tree orig; > + > +public: > + c_omp_address_inspector (location_t loc, tree t) > + : loc (loc), root_term (NULL_TREE), indirections (false), > + map_supported (-1), orig (t) > + { } > + > + ~c_omp_address_inspector () {} > + > + virtual bool processing_template_decl_p () { return false; } > + virtual bool mappable_type (tree t); > + virtual void emit_unmappable_type_notes (tree) { } > + > + bool check_clause (tree); > + tree get_root_term (bool); > + > + tree get_address () { return orig; } > + tree get_deref_origin (); > + bool component_access_p (); > + > + bool has_indirections_p () > + { > + if (!root_term) > + get_root_term (false); > + return indirections; > + } > + > + bool indir_component_ref_p () > + { > + return component_access_p () && has_indirections_p (); > + } I think https://gcc.gnu.org/codingconventions.html#Cxx_Conventions just says that no member functions should be defined inside of the class, which is something that almost nobody actually honors. But, when they are inline, there should be one style, not many, so either type method (args) { } (guess my preference) or type method (args) { } but not those mixed up, which you have in the patch. > --- a/gcc/c-family/c-omp.cc > +++ b/gcc/c-family/c-omp.cc > @@ -3113,6 +3113,274 @@ c_omp_adjust_map_clauses (tree clauses, bool is_target) > } > } > There should be function comment for all the out of line definitions. > +tree > +c_omp_address_inspector::get_deref_origin () > { > if (error_operand_p (t)) > return error_mark_node; > + c_omp_address_inspector t_insp (OMP_CLAUSE_LOCATION (c), t); Wouldn't ai (address inspector) be better than t_insp? > +/* C++ specialisation of the c_omp_address_inspector class. */ > + > +class cp_omp_address_inspector : public c_omp_address_inspector > +{ > +public: > + cp_omp_address_inspector (location_t loc, tree t) > + : c_omp_address_inspector (loc, t) > + { } > + > + ~cp_omp_address_inspector () > + { } > + > + bool processing_template_decl_p () > + { > + return processing_template_decl; > + } > + > + bool mappable_type (tree t) > + { > + return cp_omp_mappable_type (t); > + } > + > + void emit_unmappable_type_notes (tree t) > + { > + cp_omp_emit_unmappable_type_notes (t); > + } > + > + static bool ref_p (tree t) > + { > + return (TYPE_REF_P (TREE_TYPE (t)) > + || REFERENCE_REF_P (t)); > + } See above the mixing of styles... I know, some headers are really bad examples, e.g. hash-map.h even has 3 different styles, { } and { } and { } for the type method (args) indented by 2 spaces. > --- /dev/null > +++ b/gcc/testsuite/g++.dg/gomp/unmappable-component-1.C > @@ -0,0 +1,21 @@ > +/* { dg-do compile } */ > + > +struct A { > + static int x[10]; > +}; > + > +struct B { > + A a; > +}; > + > +int > +main (int argc, char *argv[]) > +{ > + B *b = new B; > +#pragma omp target map(b->a) // { dg-error "'b->B::a' does not have a mappable type in 'map' clause" } > + ; > + B bb; > +#pragma omp target map(bb.a) // { dg-error "'bb\.B::a' does not have a mappable type in 'map' clause" } We don't diagnose static data members as non-mappable anymore. So I don't see how this test can work. > +int > +main (int argc, char *argv[]) Why "int argc, char *argv[]" when you don't use it? > + p0 = (p0type *) malloc (sizeof *p0); > + p0->x0[0].p1 = (p1type *) malloc (sizeof *p0->x0[0].p1); > + p0->x0[0].p1->p2 = (p2type *) malloc (sizeof *p0->x0[0].p1->p2); > + memset (p0->x0[0].p1->p2, 0, sizeof *p0->x0[0].p1->p2); > + > +#pragma omp target map(tofrom: p0->x0[k1].p1->p2[k2].x1[k3].x2[4][0:n]) \ > + map(to: p0->x0[k1].p1, p0->x0[k1].p1->p2) \ > + map(to: p0->x0[k1].p1[0]) > + { > + for (int i = 0; i < n; i++) > + p0->x0[k1].p1->p2[k2].x1[k3].x2[4][i] = i; > + } > + > + for (int i = 0; i < n; i++) > + assert (i == p0->x0[k1].p1->p2[k2].x1[k3].x2[4][i]); It would be nice to free pointers afterwards in order not to give bad coding examples to people. Otherwise LGTM. Jakub