On Thu, Nov 05, 2020 at 01:03:38PM +0100, Tobias Burnus wrote:
> OpenACC piggybacks on OpenACC for the atomic parsing; however, there
> are two issues:
> * Newer OpenMP versions added additional clauses such as 'seq_cst',
> which do not exist in OpenACC.
> * OpenACC 2.6 added 'acc atomic update capture' (besides 'acc atomic capture',
> which was not accepted.
>
> Actually, while OpenACC 2.6/2.7/3.0 has 'acc atomic update capture' in the
> syntax, it never explicitly states that this matches 'atomic capture'.
>
> NOTE: I did not check whether the supported expressions by OpenMP 5.0/the
> current GCC implementation is the same as in OpenACC 2.6/2.7/3.x.
>
> Any comments?
> OpenACC (C/C++): Fix 'acc atomic' parsing
>
> gcc/c/ChangeLog:
>
> * c-parser.c (c_parser_omp_atomic): Add openacc parameter and update
> OpenACC matching.
> (c_parser_omp_construct): Update call.
>
> gcc/cp/ChangeLog:
>
> * parser.c (cp_parser_omp_atomic): Add openacc parameter and update
> OpenACC matching.
> (cp_parser_omp_construct): Update call.
>
> gcc/testsuite/ChangeLog:
>
> * c-c++-common/goacc-gomp/atomic.c: New test.
> * c-c++-common/goacc/atomic.c: New test.
>
> gcc/c/c-parser.c | 40 +++++++++++++++---------
> gcc/cp/parser.c | 39 ++++++++++++++---------
> gcc/testsuite/c-c++-common/goacc-gomp/atomic.c | 43
> ++++++++++++++++++++++++++
> gcc/testsuite/c-c++-common/goacc/atomic.c | 30 ++++++++++++++++++
> 4 files changed, 124 insertions(+), 28 deletions(-)
>
> diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
> index fc97aa3f95f..79037d98f76 100644
> --- a/gcc/c/c-parser.c
> +++ b/gcc/c/c-parser.c
> @@ -17304,7 +17304,7 @@ c_parser_oacc_wait (location_t loc, c_parser *parser,
> char *p_name)
> LOC is the location of the #pragma token. */
>
> static void
> -c_parser_omp_atomic (location_t loc, c_parser *parser)
> +c_parser_omp_atomic (location_t loc, c_parser *parser, bool openacc)
> {
> tree lhs = NULL_TREE, rhs = NULL_TREE, v = NULL_TREE;
> tree lhs1 = NULL_TREE, rhs1 = NULL_TREE;
> @@ -17343,17 +17343,17 @@ c_parser_omp_atomic (location_t loc, c_parser
> *parser)
> new_code = OMP_ATOMIC;
> else if (!strcmp (p, "capture"))
> new_code = OMP_ATOMIC_CAPTURE_NEW;
> - else if (!strcmp (p, "seq_cst"))
> + else if (!openacc && !strcmp (p, "seq_cst"))
> new_memory_order = OMP_MEMORY_ORDER_SEQ_CST;
> - else if (!strcmp (p, "acq_rel"))
> + else if (!openacc && !strcmp (p, "acq_rel"))
> new_memory_order = OMP_MEMORY_ORDER_ACQ_REL;
> - else if (!strcmp (p, "release"))
> + else if (!openacc && !strcmp (p, "release"))
> new_memory_order = OMP_MEMORY_ORDER_RELEASE;
> - else if (!strcmp (p, "acquire"))
> + else if (!openacc && !strcmp (p, "acquire"))
> new_memory_order = OMP_MEMORY_ORDER_ACQUIRE;
> - else if (!strcmp (p, "relaxed"))
> + else if (!openacc && !strcmp (p, "relaxed"))
> new_memory_order = OMP_MEMORY_ORDER_RELAXED;
> - else if (!strcmp (p, "hint"))
> + else if (!openacc && !strcmp (p, "hint"))
> {
> c_parser_consume_token (parser);
> clauses = c_parser_omp_clause_hint (parser, clauses);
> @@ -17362,15 +17362,24 @@ c_parser_omp_atomic (location_t loc, c_parser
> *parser)
> else
> {
> p = NULL;
> - error_at (cloc, "expected %<read%>, %<write%>, %<update%>, "
> - "%<capture%>, %<seq_cst%>, %<acq_rel%>, "
> - "%<release%>, %<relaxed%> or %<hint%> clause");
> + if (openacc)
> + error_at (cloc, "expected %<read%>, %<write%>, %<update%>, "
> + "or %<capture%> clause");
> + else
> + error_at (cloc, "expected %<read%>, %<write%>, %<update%>, "
> + "%<capture%>, %<seq_cst%>, %<acq_rel%>, "
> + "%<release%>, %<relaxed%> or %<hint%> clause");
Wouldn't it be much simpler and more readable to do:
else if (!strcmp (p, "capture"))
new_code = OMP_ATOMIC_CAPTURE_NEW;
+ else if (openacc)
+ {
+ p = NULL;
+ error_at (cloc, "expected %<read%>, %<write%>, %<update%>, "
+ "or %<capture%> clause");
+ }
else if (!strcmp (p, "seq_cst"))
new_memory_order = OMP_MEMORY_ORDER_SEQ_CST;
... - handling of other openmp only clauses here
else
{
p = NULL;
error_at (cloc, "expected %<read%>, %<write%>, %<update%>, "
"%<capture%>, %<seq_cst%>, %<acq_rel%>, "
"%<release%>, %<relaxed%> or %<hint%> clause");
}
?
Ditto C++.
Otherwise LGTM, but I have no idea what OpenACC actually says...
Jakub
