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.133.124]) by sourceware.org (Postfix) with ESMTP id 5C9E0385842D for ; Wed, 6 Oct 2021 15:36:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5C9E0385842D 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-320-41rbKvRsOv-v2M8VcZxMrA-1; Wed, 06 Oct 2021 11:36:42 -0400 X-MC-Unique: 41rbKvRsOv-v2M8VcZxMrA-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B65521023F53; Wed, 6 Oct 2021 15:36:40 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.193.109]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 542E95D9DD; Wed, 6 Oct 2021 15:36:39 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.16.1/8.16.1) with ESMTPS id 196Fab2g3920839 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Wed, 6 Oct 2021 17:36:37 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.16.1/8.16.1/Submit) id 196FaaGR3920838; Wed, 6 Oct 2021 17:36:36 +0200 Date: Wed, 6 Oct 2021 17:36:35 +0200 From: Jakub Jelinek To: Kwok Cheung Yeung Cc: gcc-patches , fortran , Tobias Burnus Subject: Re: [PATCH] openmp, fortran: Add support for declare variant in Fortran Message-ID: <20211006153635.GJ304296@tucnak> Reply-To: Jakub Jelinek References: <8296c73e-2a16-0232-d1ac-49c5c7330481@codesourcery.com> MIME-Version: 1.0 In-Reply-To: <8296c73e-2a16-0232-d1ac-49c5c7330481@codesourcery.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 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=-5.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Wed, 06 Oct 2021 15:36:44 -0000 On Wed, Oct 06, 2021 at 12:39:01PM +0100, Kwok Cheung Yeung wrote: > --- a/gcc/gimplify.c > +++ b/gcc/gimplify.c > @@ -11599,8 +11599,11 @@ omp_construct_selector_matches (enum tree_code *constructs, int nconstructs, > } > } > if (!target_seen > - && lookup_attribute ("omp declare target block", > - DECL_ATTRIBUTES (current_function_decl))) > + && (lookup_attribute ("omp declare target block", > + DECL_ATTRIBUTES (current_function_decl)) > + || (lang_GNU_Fortran () > + && lookup_attribute ("omp declare target", > + DECL_ATTRIBUTES (current_function_decl))))) > { > if (scores) > codes.safe_push (OMP_TARGET); So, as I wrote in the other mail, my preference would be to drop this hunk and adjust testcases appropriately (perhaps with a comment). The 5.1 way will then be different for all 3 languages and 5.2 way as well. > + const char *str = TREE_STRING_POINTER (TREE_VALUE (t2)); > + if (!strcmp (str, props[i].props[j]) > + && ((size_t) TREE_STRING_LENGTH (TREE_VALUE (t2)) > + == strlen (str) + (lang_GNU_Fortran () ? 0 : 1))) This is a little bit strange but if all identifiers from Fortran FE behave that way and differently from C/C++ FEs, I guess we can live with that (multiple occurrences thereof). > + DECL_ATTRIBUTES (base_fn_decl) > + = tree_cons ( > + get_identifier ("omp declare variant base"), > + build_tree_list (gfc_get_symbol_decl (variant_proc_sym), > + set_selectors), > + DECL_ATTRIBUTES (base_fn_decl)); Perhaps that is just my private coding convention preference, but I really don't like these calls with function ( on one line and less indented arguments on another one. I'd find it more readable to do use temporaries where possible, tree id = get_identifier ("omp declare variant base"); tree var = gfc_get_symbol_decl (variant_proc_sym); DECL_ATTRIBUTES (base_fn_decl) = tree_cons (id, build_tree_list (var, set_selectors), DECL_ATTRIBUTES (base_fn_decl)); is IMHO more radable and fits on fewer lines even. Other than that the patch looks mostly good, what I miss in the testcases though is Fortran specific stuff, e.g. I couldn't find a single testcase that uses the !$omp declare variant (procname:variantprocname) match (construct={parallel}) syntax and couldn't find testcase coverage or resolving of the Fortran specific declare variant restrictions. See OpenMP 5.0 [60:1-12], which includes base-proc-name must not be a generic name, procedure pointer, or entry name. and If base-proc-name is omitted then the declare variant directive must appear in the specification part of a subroutine subprogram or a function subprogram. etc. So unless I've completely missed that in the patch somewhere, please try to add new testcases (i.e. with no c-c++-common counterparts) that test all those restrictions in there, have one !$omp declare variant with base-proc-name that is a generic name and dg-error for it, another one for procedure pointer, another one for entry name, another one for !$omp declare variant with base-proc-name omitted that appears where it isn't allowed, some !$omp declare variant (both with and without proc-base-name) that appears e.g. in execution part, etc. My Fortran knowledge is rusty, but I hope Tobias could help there if needed, and if some of the restrictions make no sense, look at what has changed in 5.1 or 5.2 current state if it hasn't been clarified. Jakub