On Mon, Jun 27, 2016 at 11:36:26AM -0700, Cesar Philippidis wrote:
> With that aside, this patch should correct the issues you pointed out.
> gfc_match_omp_clauses now uses a common function to parse the gang,
> worker and vector clauses. Also, this patch takes extra care with
> MATCH_ERRORs when parsing the wait and async clauses and the wait
> directive. One oddity I noticed is how the fortran FE accepts directives
> with clauses specified like this
>
> !$omp parallel if(.true.)private(x)
That is completely intentional, in that case the whitespace isn't needed to
separate tokens. It is only a matter of coding style then.
Somebody will only use
!$omp parallel if(.true.) private(x)
other people
!$omp parallel if ( .true. ) , private ( x )
etc.
> Shouldn't there be some sort of separator between if and private? I know
> that it's easy to distinguish the clauses because of the parenthesis,
> but it still looks weird.
If user wants, it can, but it isn't required by the standard.
In C you can also write if(cond)if(cond2)if(cond3){foo(x,y,z);}else{bar(x);}
if you want, similarly Fortran. OpenMP/Fortran even allows
!$omp endparalleldosimd
and similar, you don't have to use spaces in between end parallel do simd.
For fixed form of course even more whitespace is optional.
> @@ -630,9 +653,10 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, uint64_t
> mask,
> {
> gfc_omp_clauses *c = gfc_get_omp_clauses ();
> locus old_loc;
> + bool seen_error = false;
>
> *cp = NULL;
> - while (1)
> + while (!seen_error)
> {
> if ((first || gfc_match_char (',') != MATCH_YES)
> && (needs_space && gfc_match_space () != MATCH_YES))
Why?
The main loop is while (1) which has break; as the last statement.
Instead of setting seen_error, just set
gfc_current_locus = old_loc;
if you have already matched successfully something, and then break;
as you already do.
> @@ -1275,9 +1309,16 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, uint64_t
> mask,
> continue;
> if ((mask & OMP_CLAUSE_TILE)
> && !c->tile_list
> - && match_oacc_expr_list ("tile (", &c->tile_list,
> - true) == MATCH_YES)
> - continue;
> + && gfc_match ("tile") == MATCH_YES)
> + {
> + if (match_oacc_expr_list (" (", &c->tile_list, true) != MATCH_YES)
> + {
> + seen_error = true;
> + break;
> + }
> + needs_space = true;
> + continue;
> + }
> if ((mask & OMP_CLAUSE_TO)
> && gfc_match_omp_variable_list ("to (",
> &c->lists[OMP_LIST_TO], false,
So, tile without ()s is also a valid clause in OpenACC?
If yes, what do you set in c structure for the existence of the clause?
If it is not valid, then the above change looks wrong.
> @@ -1309,10 +1350,13 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, uint64_t
> mask,
> && gfc_match ("vector") == MATCH_YES)
> {
> c->vector = true;
> - if (gfc_match (" ( length : %e )", &c->vector_expr) == MATCH_YES
> - || gfc_match (" ( %e )", &c->vector_expr) == MATCH_YES)
> - needs_space = false;
> - else
> + match m = match_oacc_clause_gwv(c, GOMP_DIM_VECTOR);
Formatting, space before (.
> @@ -1348,7 +1402,7 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, uint64_t
> mask,
> break;
> }
>
> - if (gfc_match_omp_eos () != MATCH_YES)
> + if (seen_error || gfc_match_omp_eos () != MATCH_YES)
> {
> gfc_free_omp_clauses (c);
> return MATCH_ERROR;
Again, IMHO not needed, if you restore the old_loc into gfc_current_loc,
then gfc_match_omp_eos () will surely fail.
Jakub
