On Thu, Jun 16, 2016 at 08:22:29PM -0700, Cesar Philippidis wrote:
> --- a/gcc/fortran/openmp.c
> +++ b/gcc/fortran/openmp.c
> @@ -677,7 +677,6 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, uint64_t
> mask,
> && gfc_match ("async") == MATCH_YES)
> {
> c->async = true;
> - needs_space = false;
> if (gfc_match (" ( %e )", &c->async_expr) != MATCH_YES)
> {
> c->async_expr
> @@ -685,6 +684,7 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, uint64_t
> mask,
> gfc_default_integer_kind,
> &gfc_current_locus);
> mpz_set_si (c->async_expr->value.integer, GOMP_ASYNC_NOVAL);
> + needs_space = true;
> }
> continue;
> }
> @@ -1328,7 +1328,8 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, uint64_t
> mask,
> && gfc_match ("wait") == MATCH_YES)
> {
> c->wait = true;
> - match_oacc_expr_list (" (", &c->wait_list, false);
> + if (match_oacc_expr_list (" (", &c->wait_list, false) == MATCH_NO)
> + needs_space = true;
> continue;
> }
> if ((mask & OMP_CLAUSE_WORKER)
I think it is still problematic. Most of the parsing fortran FE errors are
deferred,
meaning that if you don't reject the whole gfc_match_omp_clauses, then no
diagnostics is actually emitted. Both
gfc_match (" ( %e )", &c->async_expr) and match_oacc_expr_list (" (",
&c->wait_list, false)
IMHO can return MATCH_YES, MATCH_NO and MATCH_ERROR, and I believe you need
to do different actions in each case.
In particular, if something is optional, then for MATCH_YES you should
accept it (continue) and not set needs_space, because after ) you don't need
space. If MATCH_NO, then you should accept it too (because it is optional),
and set needs_space = true; first and perhaps do whatever else you need to
do. If MATCH_ERROR, then you should make sure not to accept it, e.g. by
doing break; or making sure continue will not be done (which one depends on
whether it might be validly parsed as some other clause, which is very
likely not the case). In the above changes, you do it all except for the
MATCH_ERROR handling, where you still do continue; and thus I bet
diagnostics for it won't be reported.
E.g. for
!$omp acc parallel async(&abc)
!$omp acc end parallel
end
no diagnostics is reported. Looking around, there are many more issues like
that, e.g. match_oacc_clause_gang(c) (note, wrong formatting) also ignores
MATCH_ERROR, etc.
> @@ -1649,7 +1650,7 @@ gfc_match_oacc_wait (void)
> gfc_expr_list *wait_list = NULL, *el;
>
> match_oacc_expr_list (" (", &wait_list, true);
> - gfc_match_omp_clauses (&c, OACC_WAIT_CLAUSES, false, false, true);
> + gfc_match_omp_clauses (&c, OACC_WAIT_CLAUSES, false, true, true);
>
> if (gfc_match_omp_eos () != MATCH_YES)
> {
Can you explain this change? I bet it again suffers from the above
mentioned issue. If match_oacc_expr_list returns MATCH_YES, I believe you
want false, false, true as you don't need space in between the closing
) of the wait_list and name of next clause. Note, does OpenACC allow also comma
in that case?
!$acc wait (whatever),async
?
If match_oacc_expr_list returns MATCH_NO, then IMHO it should be
true, true, true, because you don't want to accept
!$acc waitasync
and also don't want to accept
!$acc wait,async
And if match_oacc_expr_list returns MATCH_ERROR, you should reject it, so
that diagnostics is emitted.
Jakub
