OpenACC: C/C++ - fix async parsing [PR99137]

For:
   #pragma acc parallel async(1,2)

avoid with C an ICE for the original tree:
   #pragma acc parallel async(<<< Unknown tree: c_maybe_const_expr 1, 2 >>>)

It did not ICE with C++, but I think the tree does not make sense, either:
   #pragma acc parallel async(<<< Unknown tree: void_cst >>>, 2)

OK for mainline?
(Not a regression; I don't know whether it makes sense to apply to other branches).

Tobias

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf

Hi!

On 2021-02-19T13:59:20+0100, Tobias Burnus <tobias@codesourcery.com> wrote:
> For:
>    #pragma acc parallel async(1,2)
>
> avoid with C an ICE for the original tree:
>    #pragma acc parallel async(<<< Unknown tree: c_maybe_const_expr 1, 2 >>>)
>
> It did not ICE with C++, but I think the tree does not make sense, either:
>    #pragma acc parallel async(<<< Unknown tree: void_cst >>>, 2)

Led by Tobias, in today's "OpenACC Technical Meeting", we discussed
<https://github.com/OpenACC/openacc-spec/issues/354> "What does
'async(1, 2)' mean?" (only visible to members of OpenACC GitHub), and
quickly came to agreement that the intention indeed is to have such
clause arguments be C/C++ base language "assignment-expressions" (like
Jakub in PR99137 had described OpenMP that does).

This will be clarified in a later OpenACC specification release.

The same issue likely also appears for other clauses (not taking a list
of expressions), so will then need to be generally reviewed -- that's for
later.

However, since Tobias has already posted a patch for 'async' (thanks!):

> OK for mainline?
> (Not a regression;

..., but does fix an ICE, and the code change is very specific, I'd say
that's good to push.

> I don't know whether it makes sense to apply to other branches).

I'd say so; I expect it to apply without any fuzz, and expect to fix the
same ICEs there.

One minor comment:

> OpenACC: C/C++ - fix async parsing [PR99137]
>
> gcc/c/ChangeLog:
>
>       PR c/99137
>       * c-parser.c (c_parser_oacc_clause_async): Reject comma expressions.
>
> gcc/cp/ChangeLog:
>
>       PR c/99137
>       * parser.c (cp_parser_oacc_clause_async): Reject comma expressions.
>
> gcc/testsuite/ChangeLog:
>
>       PR c/99137
>       * c-c++-common/goacc/asyncwait-1.c: Update dg-error.
>       * c-c++-common/goacc/async-1.c: New test.
>
>  gcc/c/c-parser.c                               |  2 +-
>  gcc/cp/parser.c                                |  2 +-
>  gcc/testsuite/c-c++-common/goacc/async-1.c     |  7 +++++++
>  gcc/testsuite/c-c++-common/goacc/asyncwait-1.c | 16 ++++++++--------
>  4 files changed, 17 insertions(+), 10 deletions(-)

I suggest to merge the new 'c-c++-common/goacc/async-1.c' into the
existing 'c-c++-common/goacc/asyncwait-1.c'; no need for an extra
testcase file?  Just one '#pragma acc wait async (1,2)' variant with
"PR99137" comment after the other "comma variants" in there, seems
sufficient?


Grüße
 Thomas


> diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
> index 2a49d07bab4..5cdeb21a458 100644
> --- a/gcc/c/c-parser.c
> +++ b/gcc/c/c-parser.c
> @@ -14332,7 +14332,7 @@ c_parser_oacc_clause_async (c_parser *parser, tree list)
>      {
>        c_parser_consume_token (parser);
>
> -      t = c_parser_expression (parser).value;
> +      t = c_parser_expr_no_commas (parser, NULL).value;
>        if (!INTEGRAL_TYPE_P (TREE_TYPE (t)))
>       c_parser_error (parser, "expected integer expression");
>        else if (t == error_mark_node
> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> index 70775792161..6a29b6dca10 100644
> --- a/gcc/cp/parser.c
> +++ b/gcc/cp/parser.c
> @@ -37991,7 +37991,7 @@ cp_parser_oacc_clause_async (cp_parser *parser, tree list)
>        matching_parens parens;
>        parens.consume_open (parser);
>
> -      t = cp_parser_expression (parser);
> +      t = cp_parser_assignment_expression (parser);
>        if (t == error_mark_node
>         || !parens.require_close (parser))
>       cp_parser_skip_to_closing_parenthesis (parser, /*recovering=*/true,
> diff --git a/gcc/testsuite/c-c++-common/goacc/async-1.c b/gcc/testsuite/c-c++-common/goacc/async-1.c
> new file mode 100644
> index 00000000000..a578dabce8c
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/goacc/async-1.c
> @@ -0,0 +1,7 @@
> +/* PR c/99137 */
> +
> +void f ()
> +{
> +  #pragma acc parallel async(1,2)  /* { dg-error "expected '\\)' before ',' token" } */
> +  ;
> +}
> diff --git a/gcc/testsuite/c-c++-common/goacc/asyncwait-1.c b/gcc/testsuite/c-c++-common/goacc/asyncwait-1.c
> index 2f5d4762b49..b5d789621ec 100644
> --- a/gcc/testsuite/c-c++-common/goacc/asyncwait-1.c
> +++ b/gcc/testsuite/c-c++-common/goacc/asyncwait-1.c
> @@ -9,7 +9,7 @@ f (int N, float *a, float *b)
>              b[ii] = a[ii];
>      }
>
> -#pragma acc parallel copyin (a[0:N]) copy (b[0:N]) async (1,) /* { dg-error "expected (primary-|)expression before" } */
> +#pragma acc parallel copyin (a[0:N]) copy (b[0:N]) async (1,) /* { dg-error "expected '\\)' before ',' token" } */
>      {
>          for (ii = 0; ii < N; ii++)
>              b[ii] = a[ii];
> @@ -21,19 +21,19 @@ f (int N, float *a, float *b)
>              b[ii] = a[ii];
>      }
>
> -#pragma acc parallel copyin (a[0:N]) copy (b[0:N]) async (1,2,) /* { dg-error "expected (primary-|)expression before" } */
> +#pragma acc parallel copyin (a[0:N]) copy (b[0:N]) async (1,2,) /* { dg-error "expected '\\)' before ',' token" } */
>      {
>          for (ii = 0; ii < N; ii++)
>              b[ii] = a[ii];
>      }
>
> -#pragma acc parallel copyin (a[0:N]) copy (b[0:N]) async (1,2 3) /* { dg-error "expected '\\)' before numeric constant" } */
> +#pragma acc parallel copyin (a[0:N]) copy (b[0:N]) async (1,2 3) /* { dg-error "expected '\\)' before ',' token" } */
>      {
>          for (ii = 0; ii < N; ii++)
>              b[ii] = a[ii];
>      }
>
> -#pragma acc parallel copyin (a[0:N]) copy (b[0:N]) async (1,2,,) /* { dg-error "expected (primary-|)expression before" } */
> +#pragma acc parallel copyin (a[0:N]) copy (b[0:N]) async (1,2,,) /* { dg-error "expected '\\)' before ',' token" } */
>      {
>          for (ii = 0; ii < N; ii++)
>              b[ii] = a[ii];
> @@ -193,15 +193,15 @@ f (int N, float *a, float *b)
>
>  #pragma acc wait async (1 2) /* { dg-error "expected '\\)' before numeric constant" } */
>
> -#pragma acc wait async (1,) /* { dg-error "expected (primary-|)expression before" } */
> +#pragma acc wait async (1,) /* { dg-error "expected '\\)' before ',' token" } */
>
>  #pragma acc wait async (,1) /* { dg-error "expected (primary-|)expression before" } */
>
> -#pragma acc wait async (1,2,) /* { dg-error "expected (primary-|)expression before" } */
> +#pragma acc wait async (1,2,) /* { dg-error "expected '\\)' before ',' token" } */
>
> -#pragma acc wait async (1,2 3) /* { dg-error "expected '\\)' before numeric constant" } */
> +#pragma acc wait async (1,2 3) /* { dg-error "expected '\\)' before ',' token" } */
>
> -#pragma acc wait async (1,2,,) /* { dg-error "expected (primary-|)expression before" } */
> +#pragma acc wait async (1,2,,) /* { dg-error "expected '\\)' before ',' token" } */
>
>  #pragma acc wait async (1 /* { dg-error "expected '\\)' before end of line" } */
>
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf