From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 87930 invoked by alias); 20 Sep 2018 14:41:18 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 87819 invoked by uid 89); 20 Sep 2018 14:41:17 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=UD:trans-decl.c, trans-decl.c, transdeclc, sk:gfc_add X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 20 Sep 2018 14:41:14 +0000 Received: from svr-orw-mbx-04.mgc.mentorg.com ([147.34.90.204]) by relay1.mentorg.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-SHA384:256) id 1g308l-0005kI-Pq from Cesar_Philippidis@mentor.com ; Thu, 20 Sep 2018 07:41:11 -0700 Received: from [127.0.0.1] (147.34.91.1) by SVR-ORW-MBX-04.mgc.mentorg.com (147.34.90.204) with Microsoft SMTP Server (TLS) id 15.0.1320.4; Thu, 20 Sep 2018 07:41:09 -0700 Subject: Re: [openacc] Teach gfortran to lower OpenACC routine dims To: Bernhard Reutner-Fischer CC: "gcc-patches@gcc.gnu.org" , Fortran List , Jakub Jelinek References: <20180920002718.6b610881@nbbrfq.cc.univie.ac.at> From: Cesar Philippidis Message-ID: Date: Thu, 20 Sep 2018 14:41:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20180920002718.6b610881@nbbrfq.cc.univie.ac.at> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-SW-Source: 2018-09/txt/msg01155.txt.bz2 On 09/19/2018 03:27 PM, Bernhard Reutner-Fischer wrote: > On Wed, 5 Sep 2018 12:52:03 -0700 > Cesar Philippidis wrote: > >> At present, gfortran does not encode the gang, worker or vector >> parallelism clauses when it creates acc routines dim attribute for >> subroutines and functions. While support for acc routine is lacking in >> other areas in gfortran (including modules), this patch is important >> because it encodes the parallelism attributes using the same function >> as the C and C++ FEs. This will become important with the forthcoming >> nvptx vector length extensions, because large vectors are not >> supported in acc routines yet. >> >> Is this OK for trunk? I regtested and bootstrapped for x86_64 with >> nvptx offloading. > >> diff --git a/gcc/fortran/openmp.c b/gcc/fortran/openmp.c >> index 94a7f7eaa50..d48c9351e25 100644 >> --- a/gcc/fortran/openmp.c >> +++ b/gcc/fortran/openmp.c >> @@ -2234,34 +2234,45 @@ gfc_match_oacc_cache (void) >> return MATCH_YES; >> } >> >> -/* Determine the loop level for a routine. */ >> +/* Determine the loop level for a routine. Returns >> OACC_FUNCTION_NONE >> + if any error is detected. */ >> >> -static int >> +static oacc_function >> gfc_oacc_routine_dims (gfc_omp_clauses *clauses) >> { >> int level = -1; >> + oacc_function ret = OACC_FUNCTION_AUTO; >> >> if (clauses) >> { >> unsigned mask = 0; >> >> if (clauses->gang) >> - level = GOMP_DIM_GANG, mask |= GOMP_DIM_MASK (level); >> + { >> + level = GOMP_DIM_GANG, mask |= GOMP_DIM_MASK (level); >> + ret = OACC_FUNCTION_GANG; >> + } >> if (clauses->worker) >> - level = GOMP_DIM_WORKER, mask |= GOMP_DIM_MASK (level); >> + { >> + level = GOMP_DIM_WORKER, mask |= GOMP_DIM_MASK (level); >> + ret = OACC_FUNCTION_WORKER; >> + } >> if (clauses->vector) >> - level = GOMP_DIM_VECTOR, mask |= GOMP_DIM_MASK (level); >> + { >> + level = GOMP_DIM_VECTOR, mask |= GOMP_DIM_MASK (level); >> + ret = OACC_FUNCTION_VECTOR; >> + } >> if (clauses->seq) >> - level = GOMP_DIM_MAX, mask |= GOMP_DIM_MASK (level); >> + { >> + level = GOMP_DIM_MAX, mask |= GOMP_DIM_MASK (level); >> + ret = OACC_FUNCTION_SEQ; >> + } >> >> if (mask != (mask & -mask)) >> - gfc_error ("Multiple loop axes specified for routine"); >> + ret = OACC_FUNCTION_NONE; >> } >> >> - if (level < 0) >> - level = GOMP_DIM_MAX; >> - >> - return level; >> + return ret; >> } >> >> match >> @@ -2272,6 +2283,8 @@ gfc_match_oacc_routine (void) >> match m; >> gfc_omp_clauses *c = NULL; >> gfc_oacc_routine_name *n = NULL; >> + oacc_function dims = OACC_FUNCTION_NONE; > > Unneeded initialisation of dims. ACK. >> + bool seen_error = false; >> >> old_loc = gfc_current_locus; >> >> @@ -2318,17 +2331,15 @@ gfc_match_oacc_routine (void) >> } >> else >> { >> - gfc_error ("Syntax error in !$ACC ROUTINE ( NAME ) at %C"); >> - gfc_current_locus = old_loc; >> - return MATCH_ERROR; >> + gfc_error ("Syntax error in !$ACC ROUTINE ( NAME ) at %L", >> &old_loc); >> + goto cleanup; >> } >> >> if (gfc_match_char (')') != MATCH_YES) >> { >> - gfc_error ("Syntax error in !$ACC ROUTINE ( NAME ) at %C, >> expecting" >> - " ')' after NAME"); >> - gfc_current_locus = old_loc; >> - return MATCH_ERROR; >> + gfc_error ("Syntax error in !$ACC ROUTINE ( NAME ) at %L, >> expecting" >> + " ')' after NAME", &old_loc); >> + goto cleanup; >> } >> } >> >> @@ -2337,26 +2348,83 @@ gfc_match_oacc_routine (void) >> != MATCH_YES)) >> return MATCH_ERROR; >> >> + /* Scan for invalid routine geometry. */ >> + dims = gfc_oacc_routine_dims (c); >> + if (dims == OACC_FUNCTION_NONE) >> + { >> + gfc_error ("Multiple loop axes specified in !$ACC ROUTINE at >> %L", >> + &old_loc); >> + >> + /* Don't abort early, because it's important to let the user >> + know of any potential duplicate routine directives. */ >> + seen_error = true; >> + } >> + else if (dims == OACC_FUNCTION_AUTO) >> + { >> + gfc_warning (0, "Expected one of %, %, >> % or " >> + "% clauses in !$ACC ROUTINE at %L", >> &old_loc); >> + dims = OACC_FUNCTION_SEQ; >> + } >> + >> if (sym != NULL) >> { >> - n = gfc_get_oacc_routine_name (); >> - n->sym = sym; >> - n->clauses = NULL; >> - n->next = NULL; >> - if (gfc_current_ns->oacc_routine_names != NULL) >> - n->next = gfc_current_ns->oacc_routine_names; >> - >> - gfc_current_ns->oacc_routine_names = n; >> + bool needs_entry = true; >> + >> + /* Scan for any repeated routine directives on 'sym' and report >> + an error if necessary. TODO: Extend this function to scan >> + for compatible DEVICE_TYPE dims. */ >> + for (n = gfc_current_ns->oacc_routine_names; n; n = n->next) >> + if (n->sym == sym) >> + { >> + needs_entry = false; >> + if (dims != gfc_oacc_routine_dims (n->clauses)) >> + { >> + gfc_error ("$!ACC ROUTINE already applied at %L", >> &old_loc); >> + goto cleanup; >> + } >> + } >> + >> + if (needs_entry) >> + { >> + n = gfc_get_oacc_routine_name (); >> + n->sym = sym; >> + n->clauses = c; >> + n->next = NULL; >> + n->loc = old_loc; >> + >> + if (gfc_current_ns->oacc_routine_names != NULL) >> + n->next = gfc_current_ns->oacc_routine_names; > > Just omit n->next = NULL above and unconditionally set ->next to current > ns' routine names. ACK. >> + >> + gfc_current_ns->oacc_routine_names = n; >> + } >> + >> + if (seen_error) >> + goto cleanup; >> } >> else if (gfc_current_ns->proc_name) >> { >> + if (gfc_current_ns->proc_name->attr.oacc_function != >> OACC_FUNCTION_NONE >> + && !seen_error) >> + { >> + gfc_error ("!$ACC ROUTINE already applied at %L", >> &old_loc); >> + goto cleanup; > > I'd move both this gfc_error and the one above to a duplicate_routine > label before the cleanup label and jump to that here and for the > identical gfc_error above. I did it this way because we have a forthcoming patch which adds support for Fortran modules, and that patch has a different set of errors. That said, I'll incorporate those changes and modify the patch. >> + } >> + >> if (!gfc_add_omp_declare_target >> (&gfc_current_ns->proc_name->attr, gfc_current_ns->proc_name->name, >> &old_loc)) >> goto cleanup; >> + >> gfc_current_ns->proc_name->attr.oacc_function >> - = gfc_oacc_routine_dims (c) + 1; >> + = seen_error ? OACC_FUNCTION_SEQ : dims; > > why can't you use dims unconditionally after branching to cleanup if > seen_error? I.e. move the seen_error check below to above the > attr.oacc_function setting? Yeah, it probably doesn't matter much if the function returns MATCH_ERROR. I'll change it. >> + >> + if (seen_error) >> + goto cleanup; >> } >> + else >> + /* Something has gone wrong. Perhaps there was a syntax error >> + in the program-stmt. */ >> + goto cleanup; >> >> if (n) >> n->clauses = c; >> diff --git a/gcc/fortran/trans-decl.c b/gcc/fortran/trans-decl.c >> index eea6b81ebfa..eed868f475b 100644 >> --- a/gcc/fortran/trans-decl.c >> +++ b/gcc/fortran/trans-decl.c >> @@ -46,6 +46,7 @@ along with GCC; see the file COPYING3. If not see >> #include "trans-stmt.h" >> #include "gomp-constants.h" >> #include "gimplify.h" >> +#include "omp-general.h" > > hmz. so the gomp-constants.h include would be redundant, but do we > really need omp-general.h? Good point. omp-general.h is required for oacc_build_routine_dims. > Doesn't this suggest to move this oacc dims lowering to trans-openmp.c > instead, please? So something like adding a new gfc_add_omp_offload_attributes to trans-openmp.c and call it from add_attributes_to_decl? >> #define MAX_LABEL_VALUE 99999 >> >> @@ -1403,16 +1404,29 @@ add_attributes_to_decl (symbol_attribute >> sym_attr, tree list) list = tree_cons (get_identifier ("omp declare >> target"), NULL_TREE, list); >> >> - if (sym_attr.oacc_function) >> + if (sym_attr.oacc_function != OACC_FUNCTION_NONE) >> { >> - tree dims = NULL_TREE; >> - int ix; >> - int level = sym_attr.oacc_function - 1; >> + omp_clause_code code = OMP_CLAUSE_ERROR; > > redundant initialization. ACK. >> + tree clause, dims; >> >> - for (ix = GOMP_DIM_MAX; ix--;) >> - dims = tree_cons (build_int_cst (boolean_type_node, ix >= >> level), >> - integer_zero_node, dims); >> + switch (sym_attr.oacc_function) >> + { >> + case OACC_FUNCTION_GANG: >> + code = OMP_CLAUSE_GANG; >> + break; >> + case OACC_FUNCTION_WORKER: >> + code = OMP_CLAUSE_WORKER; >> + break; >> + case OACC_FUNCTION_VECTOR: >> + code = OMP_CLAUSE_VECTOR; >> + break; >> + case OACC_FUNCTION_SEQ: >> + default: >> + code = OMP_CLAUSE_SEQ; >> + } >> >> + clause = build_omp_clause (UNKNOWN_LOCATION, code); >> + dims = oacc_build_routine_dims (clause); >> list = tree_cons (get_identifier ("oacc function"), >> dims, list); >> } On a related note, I noticed that I forgot to incorporate this change in gfortran.h: @@ -902,7 +912,7 @@ typedef struct unsigned oacc_declare_link:1; /* This is an OpenACC acclerator function at level N - 1 */ - unsigned oacc_function:3; + ENUM_BITFIELD (oacc_function) oacc_function:3; It's probably not huge, but I noticed that some other enum bitfields are declared that way. > btw.. the OACC merge from the gomp4 branch added a copy'n paste error > in an error message. May i ask you to regtest and install the below: > > diff --git a/gcc/fortran/openmp.c b/gcc/fortran/openmp.c > index fcfe671be8b..ac1f4fc7619 100644 > --- a/gcc/fortran/openmp.c > +++ b/gcc/fortran/openmp.c > @@ -5848,13 +5848,13 @@ resolve_oacc_loop_blocks (gfc_code *code) > if (c->code->ext.omp_clauses->worker) > gfc_error ("Loop parallelized across gangs is not > allowed " "inside loop parallelized across workers at %L", > &code->loc); > if (c->code->ext.omp_clauses->vector) > gfc_error ("Loop parallelized across gangs is not > allowed " > - "inside loop parallelized across workers > at %L", > + "inside loop parallelized across vectors > at %L", &code->loc); > } > if (code->ext.omp_clauses->worker) > { > if (c->code->ext.omp_clauses->worker) > gfc_error ("Loop parallelized across workers is not > allowed " Sure. That looks reasonable. I'll also update and/or add new tests as necessary. Thanks for the review. I have couple in my queue already, but I hope to have both my updated patch and your patch ready early next week. This week I've been preparing a bunch of miscellaneous OpenACC patches, but next week return to OpenACC routine patches. In terms of Fortran, I have a patch that introduces support for the nohost routine clause (and the bind clause, but bind hasn't been implemented in the middle end yet, so I won't include it), as well as a patch the aforementioned routine support in Fortran modules. Cesar