From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 37916 invoked by alias); 4 Dec 2018 12:57:33 -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 37889 invoked by uid 89); 4 Dec 2018 12:57:32 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=00am, ort X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 04 Dec 2018 12:57:29 +0000 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 mx1.redhat.com (Postfix) with ESMTPS id A603B3082E46; Tue, 4 Dec 2018 12:57:28 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-117-214.ams2.redhat.com [10.36.117.214]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 47E485DE01; Tue, 4 Dec 2018 12:57:28 +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 wB4CvPSZ024499; Tue, 4 Dec 2018 13:57:26 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id wB4CvORJ024498; Tue, 4 Dec 2018 13:57:24 +0100 Date: Tue, 04 Dec 2018 12:57:00 -0000 From: Jakub Jelinek To: Cesar Philippidis Cc: "gcc-patches@gcc.gnu.org" , Tom de Vries , Fortran List Subject: Re: [patch] various OpenACC reduction enhancements - FE changes Message-ID: <20181204125724.GL12380@tucnak> Reply-To: Jakub Jelinek References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2018-12/txt/msg00166.txt.bz2 On Fri, Jun 29, 2018 at 11:22:00AM -0700, Cesar Philippidis wrote: > 2018-06-29 Cesar Philippidis > Nathan Sidwell > > gcc/c/ > * c-parser.c (c_parser_omp_variable_list): New c_omp_region_type > argument. Use it to specialize handling of OMP_CLAUSE_REDUCTION for > OpenACC. > (c_parser_omp_clause_reduction): Update call to > c_parser_omp_variable_list. Propage OpenACC errors as necessary. > (c_parser_oacc_all_clauses): Update call to > p_parser_omp_clause_reduction. > (c_parser_omp_all_clauses): Likewise. > * c-typeck.c (c_finish_omp_clauses): Emit an error on orphan OpenACC > gang reductions. > > gcc/cp/ > * parser.c (cp_parser_omp_var_list_no_open): New c_omp_region_type > argument. Use it to specialize handling of OMP_CLAUSE_REDUCTION for > OpenACC. > (cp_parser_omp_clause_reduction): Update call to > cp_parser_omp_variable_list. Propage OpenACC errors as necessary. > (cp_parser_oacc_all_clauses): Update call to > cp_parser_omp_clause_reduction. > (cp_parser_omp_all_clauses): Likewise. > * semantics.c (finish_omp_clauses): Emit an error on orphan OpenACC > gang reductions. > > gcc/fortran/ > * openmp.c (resolve_oacc_loop_blocks): Emit an error on orphan OpenACC > gang reductions. > * trans-openmp.c (gfc_omp_clause_copy_ctor): Permit reductions. > > diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c > index 7a926285f3a..a6f453dae54 100644 > --- a/gcc/c/c-parser.c > +++ b/gcc/c/c-parser.c > @@ -965,12 +965,13 @@ class token_pair > > /* Like token_pair::require_close, except that tokens will be skipped > until the desired token is found. An error message is still produced > - if the next token is not as expected. */ > + if the next token is not as expected, unless QUIET is set. */ > > - void skip_until_found_close (c_parser *parser) const > + void skip_until_found_close (c_parser *parser, bool quiet = false) const > { > c_parser_skip_until_found (parser, traits_t::close_token_type, > - traits_t::close_gmsgid, m_open_loc); > + quiet ? NULL : traits_t::close_gmsgid, > + m_open_loc); > } I don't like these changes, why do you need them? C++ FE doesn't have such changes either, and it is fine to diagnose missing ) even if there was some earlier error. All other spots which require matching parens do it the same. Please leave those out. static tree -c_parser_omp_clause_reduction (c_parser *parser, tree list) +c_parser_omp_clause_reduction (c_parser *parser, tree list, + enum c_omp_region_type ort) Note, the signature is now different, it is ok to replace is_omp argument with enum c_omp_region_type if you wish. > { > location_t clause_loc = c_parser_peek_token (parser)->location; > + bool seen_error = false; > + > matching_parens parens; > if (parens.require_open (parser)) > { > @@ -12855,7 +12876,13 @@ c_parser_omp_clause_reduction (c_parser *parser, tree list) > tree nl, c; > > nl = c_parser_omp_variable_list (parser, clause_loc, > - OMP_CLAUSE_REDUCTION, list); > + OMP_CLAUSE_REDUCTION, list, ort); > + if (c_parser_peek_token (parser)->type != CPP_CLOSE_PAREN) > + { > + seen_error = true; > + goto cleanup; > + } > + > for (c = nl; c != list; c = OMP_CLAUSE_CHAIN (c)) > { > tree d = OMP_CLAUSE_DECL (c), type; > @@ -12891,7 +12918,8 @@ c_parser_omp_clause_reduction (c_parser *parser, tree list) > > list = nl; > } > - parens.skip_until_found_close (parser); > + cleanup: > + parens.skip_until_found_close (parser, seen_error); > } > return list; > } And the above hunks as well. > @@ -13998,7 +14026,7 @@ c_parser_oacc_all_clauses (c_parser *parser, omp_clause_mask mask, > c_name = "private"; > break; > case PRAGMA_OACC_CLAUSE_REDUCTION: > - clauses = c_parser_omp_clause_reduction (parser, clauses); > + clauses = c_parser_omp_clause_reduction (parser, clauses, C_ORT_ACC); > c_name = "reduction"; > break; > case PRAGMA_OACC_CLAUSE_SEQ: > @@ -14157,7 +14185,7 @@ c_parser_omp_all_clauses (c_parser *parser, omp_clause_mask mask, > c_name = "private"; > break; > case PRAGMA_OMP_CLAUSE_REDUCTION: > - clauses = c_parser_omp_clause_reduction (parser, clauses); > + clauses = c_parser_omp_clause_reduction (parser, clauses, C_ORT_OMP); > c_name = "reduction"; > break; > case PRAGMA_OMP_CLAUSE_SCHEDULE: Note, there are now also the IN_REDUCTION/TASK_REDUCTION clause cases that need adjustment. > diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c > index 90ae306c99a..944db3fa8be 100644 > --- a/gcc/c/c-typeck.c > +++ b/gcc/c/c-typeck.c > @@ -13087,6 +13087,14 @@ c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort) > goto check_dup_generic; > > case OMP_CLAUSE_REDUCTION: > + if (ort == C_ORT_ACC && oacc_get_fn_attrib (current_function_decl) > + && omp_find_clause (clauses, OMP_CLAUSE_GANG)) This is expensive if there are many clauses, we want to avoid O(n^2) behavior. For C we only have one loop, so just remember in some variable whether there are reduction clause(s) that would conflict with gang, and in another one whether gang clause has been seen, then deal with it at the end if both the bools are true. > + { > + error_at (OMP_CLAUSE_LOCATION (c), > + "gang reduction on an orphan loop"); > + remove = true; > + break; > + } > need_implicitly_determined = true; > t = OMP_CLAUSE_DECL (c); > if (TREE_CODE (t) == TREE_LIST) > @@ -31668,6 +31669,21 @@ cp_parser_omp_var_list_no_open (cp_parser *parser, enum omp_clause_code kind, > /* FALLTHROUGH. */ > case OMP_CLAUSE_DEPEND: > case OMP_CLAUSE_REDUCTION: > + if (kind == OMP_CLAUSE_REDUCTION && ort == C_ORT_ACC) > + { > + switch (cp_lexer_peek_token (parser->lexer)->type) > + { > + case CPP_OPEN_PAREN: > + case CPP_OPEN_SQUARE: > + case CPP_DOT: > + case CPP_DEREF: > + error ("invalid reduction variable"); > + decl = error_mark_node; > + goto skip_comma; > + default:; > + break; > + } > + } Any reason for the above (ditto in C), rather than just adding && ort != C_ORT_ACC to the while loop condition for CPP_OPEN_SQUARE? (, . or * after id-expression is like any other unhandled characters... > while (cp_lexer_next_token_is (parser->lexer, CPP_OPEN_SQUARE)) > { > tree low_bound = NULL_TREE, length = NULL_TREE; > @@ -33868,7 +33885,7 @@ cp_parser_oacc_all_clauses (cp_parser *parser, omp_clause_mask mask, > c_name = "private"; > break; > case PRAGMA_OACC_CLAUSE_REDUCTION: > - clauses = cp_parser_omp_clause_reduction (parser, clauses); > + clauses = cp_parser_omp_clause_reduction (parser, clauses, C_ORT_ACC); > c_name = "reduction"; > break; > case PRAGMA_OACC_CLAUSE_SEQ: > @@ -34055,7 +34072,7 @@ cp_parser_omp_all_clauses (cp_parser *parser, omp_clause_mask mask, > c_name = "private"; > break; > case PRAGMA_OMP_CLAUSE_REDUCTION: > - clauses = cp_parser_omp_clause_reduction (parser, clauses); > + clauses = cp_parser_omp_clause_reduction (parser, clauses, C_ORT_OMP); > c_name = "reduction"; > break; > case PRAGMA_OMP_CLAUSE_SCHEDULE: Again, needs adjustement for IN_REDUCTION/TASK_REDUCTION. > diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c > index c779137da45..177acdd9cc4 100644 > --- a/gcc/cp/semantics.c > +++ b/gcc/cp/semantics.c > @@ -5875,6 +5875,14 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) > field_ok = ((ort & C_ORT_OMP_DECLARE_SIMD) == C_ORT_OMP); > goto check_dup_generic; > case OMP_CLAUSE_REDUCTION: > + if (ort == C_ORT_ACC && oacc_get_fn_attrib (current_function_decl) > + && omp_find_clause (clauses, OMP_CLAUSE_GANG)) > + { > + error_at (OMP_CLAUSE_LOCATION (c), > + "gang reduction on an orphan loop"); > + remove = true; > + break; > + } > field_ok = ((ort & C_ORT_OMP_DECLARE_SIMD) == C_ORT_OMP); > t = OMP_CLAUSE_DECL (c); > if (TREE_CODE (t) == TREE_LIST) In C++ finish_omp_clauses there are 2 loops, so you can easily just remember if OMP_CLAUSE_GANG has been seen in the first loop and diagnose this in the second loop only. Jakub