Re: [patch] various OpenACC reduction enhancements - FE changes
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_REDUCTIO
Re: [patch] various OpenACC reduction enhancements - FE changes
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_om
Re: [patch] various OpenACC reduction enhancements - FE changes
On Thu, Dec 13, 2018 at 02:11:31PM +, Julian Brown wrote:
> > 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.
> @@ -12004,7 +12005,8 @@ c_parser_omp_variable_list (c_parser *parser,
> case OMP_CLAUSE_REDUCTION:
> case OMP_CLAUSE_IN_REDUCTION:
> case OMP_CLAUSE_TASK_REDUCTION:
> - while (c_parser_next_token_is (parser, CPP_OPEN_SQUARE))
> + while (ort != C_ORT_ACC
> + && c_parser_next_token_is (parser, CPP_OPEN_SQUARE))
> {
> tree low_bound = NULL_TREE, length = NULL_TREE;
>
> @@ -12074,6 +12076,10 @@ c_parser_omp_variable_list (c_parser *parser,
> }
> }
> }
> + if (ort == C_ORT_ACC
> + && c_parser_next_token_is_not (parser, CPP_COMMA)
> + && c_parser_next_token_is_not (parser, CPP_CLOSE_PAREN))
> + t = error_mark_node;
> break;
> default:
> break;
I still don't understand this at all, sorry.
So, t is guaranteed to be non-error_mark_node before entering this spot.
If you have reduction (decl[0]) etc. vs. reduction (decl), why do you care
whether
it is added to the returned list or not for error recovery? If it is something
that causes ICE in the gimplifier, then user could have written just
reduction (decl) or reduction (decl, ) and have it added to the list anyway,
so the bug would be that it isn't diagnosed as something incorrect in
c_finish_omp_clauses (or whatever the problem with it is).
If there is any kind of garbage after the decl, it will just return to the
caller at that point and the caller should do the error recovery, the same
for reduction (decl[0]) as well as for reduction (decl, [0]).
Jakub
Re: [patch] various OpenACC reduction enhancements - FE changes
Attaches are the FE changes for the OpenACC reduction enhancements. It
depends on the ME patch.
Is this patch OK for trunk? It bootstrapped / regression tested cleanly
for x86_64 with nvptx offloading.
Thanks,
Cesar
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);
}
private:
@@ -11498,7 +11499,8 @@ c_parser_oacc_wait_list (c_parser *parser, location_t clause_loc, tree list)
static tree
c_parser_omp_variable_list (c_parser *parser,
location_t clause_loc,
- enum omp_clause_code kind, tree list)
+ enum omp_clause_code kind, tree list,
+ enum c_omp_region_type ort = C_ORT_OMP)
{
if (c_parser_next_token_is_not (parser, CPP_NAME)
|| c_parser_peek_token (parser)->id_kind != C_ID_ID)
@@ -11557,6 +11559,22 @@ c_parser_omp_variable_list (c_parser *parser,
/* FALLTHROUGH */
case OMP_CLAUSE_DEPEND:
case OMP_CLAUSE_REDUCTION:
+ if (kind == OMP_CLAUSE_REDUCTION && ort == C_ORT_ACC)
+ {
+ switch (c_parser_peek_token (parser)->type)
+ {
+ case CPP_OPEN_PAREN:
+ case CPP_OPEN_SQUARE:
+ case CPP_DOT:
+ case CPP_DEREF:
+ error ("invalid reduction variable");
+ t = error_mark_node;
+ default:;
+ break;
+ }
+ if (t == error_mark_node)
+ break;
+ }
while (c_parser_next_token_is (parser, CPP_OPEN_SQUARE))
{
tree low_bound = NULL_TREE, length = NULL_TREE;
@@ -12789,9 +12807,12 @@ c_parser_omp_clause_private (c_parser *parser, tree list)
identifier */
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)
{
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;
}
@@ -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 P
