From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9543 invoked by alias); 19 Sep 2018 22:27:58 -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 9509 invoked by uid 89); 19 Sep 2018 22:27:57 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,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=seq, sk:build_o, Doesnt, Doesn't X-HELO: mail-wm1-f66.google.com Received: from mail-wm1-f66.google.com (HELO mail-wm1-f66.google.com) (209.85.128.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 19 Sep 2018 22:27:55 +0000 Received: by mail-wm1-f66.google.com with SMTP id r1-v6so10553864wmh.0; Wed, 19 Sep 2018 15:27:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=J9zdLDNiRaWgicEdhFOIhcf8Uukfi+G2j8KfzWnVoRc=; b=Ko07aOLK0DterOUVNpoEITZhEhOcAtHbDVk8kCEyVhMxuxzSYHoPT8tlALvbHEAoDp xV53qm/Ct0sGFlst4hdl+NctYstRO8nC0aqFVe147CEN4FygCM4KQvCuz0lC5TnS0/BC ZANWzocnjB2ZFP0Pi1v4sdw8V6zvW9T8yisB/NlmP+5Pvz4eWLlXYjFaAWJz26hFLXob dqbRW6wVn7M9xiTORdQJj8T2ARTBSr7P2YiYNvwV7ToTVMelWnnY1o0QRfT4UbVmcKhe rV9Cvboh4mOWYBGyeVfzzBPGxOQIdp51FwGWNF9ZQiRT+I8uPhvB0P+7zTRNHyy8qWnl RAsg== Return-Path: Received: from nbbrfq.cc.univie.ac.at (91-119-125-11.dsl.dynamic.surfer.at. [91.119.125.11]) by smtp.gmail.com with ESMTPSA id j75-v6sm51149wmj.8.2018.09.19.15.27.52 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 19 Sep 2018 15:27:53 -0700 (PDT) Date: Wed, 19 Sep 2018 22:30:00 -0000 From: Bernhard Reutner-Fischer To: Cesar Philippidis Cc: "gcc-patches@gcc.gnu.org" , Fortran List , Jakub Jelinek Subject: Re: [openacc] Teach gfortran to lower OpenACC routine dims Message-ID: <20180920002718.6b610881@nbbrfq.cc.univie.ac.at> In-Reply-To: References: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-09/txt/msg01105.txt.bz2 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. > + 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. > + > + 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. > + } > + > 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? > + > + 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? Doesn't this suggest to move this oacc dims lowering to trans-openmp.c instead, please? > > #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. > + 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); > } 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 " thanks,