On Tue, 4 Dec 2018 13:57:24 +0100 Jakub Jelinek wrote: > 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. I've removed these bits. > 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. I've done as you suggest. > > { > > 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. Removed. > > @@ -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. Done. > > 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. There are (now) two loops in the C frontend too -- so I've handled this the same as C++, below, because there may be multiple reduction clauses with a "gang" so removing them afterwards (like nogroup_seen) seems awkward. > > + { > > + 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... I think the reason was that 'decl' ('t' in the C version) is not set to error_mark_node if the while loop is skipped, and then the gimplifier gets confused. I've tried to tackle this in another way, by checking there aren't any stray characters before the next comma or close-parenthesis. I'm not sure if you were objecting to the error message too -- with the current patch, the user will just get e.g.: error: expected ')' before '.' token if they try to use an unsupported type of construct as a reduction target. > > 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. Done. > > 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. Done. Re-tested with offloading to nvptx, and with updates to the new testcases (to be posted). OK? Thanks, Julian ChangeLog 2018-xx-xx Cesar Philippidis Nathan Sidwell Julian Brown 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): Change is_omp boolean parameter to c_omp_region_type. Update call to c_parser_omp_variable_list. (c_parser_oacc_all_clauses): Update calls to c_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): Change is_omp boolean parameter to c_omp_region_type. Update call to cp_parser_omp_var_list_no_open. (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.