On Fri, Jul 15, 2016 at 09:44:25AM +0200, Thomas Schwinge wrote:
> Hi!
>
> On Thu, 14 Jul 2016 19:05:52 -0700, Cesar Philippidis
> <ce...@codesourcery.com> wrote:
> > The fortran FE is currently scanning for the vector clause before
> > vector_length. That's a problem match_oacc_clause_gwv matches 'vector'
> > without looking for whatever follows it. The correction I made here was
> > to scan for vector_length before vector.
>
> Does that me we (erroneously) accept any random junk after "vector",
> "vector_length", and likely other clauses?
No. The thing is, for clauses where the initial condition starts with
gfc_match ("clause_name (") and longer, we can try to match them in any
order and thus using alphabetical order is ok.
If it is like for a bunch of OpenACC clauses with optional arguments
matched just with gfc_match ("clause_name"), then matching order matters.
vector_length ( arg ) in the source right now would satisfy
&& gfc_match ("vector") == MATCH_YES
and then match_oacc_clause_gwv should yield MATCH_NO, which sets
needs_space = true; and thus we reject it later on, because there is no
whitespace nor comma after vector (there is underscore).
>
> > --- a/gcc/fortran/openmp.c
> > +++ b/gcc/fortran/openmp.c
> > @@ -1338,6 +1338,11 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp,
> > uint64_t mask,
> > continue;
> > break;
> > case 'v':
> > + if ((mask & OMP_CLAUSE_VECTOR_LENGTH)
> > + && c->vector_length_expr == NULL
> > + && (gfc_match ("vector_length ( %e )", &c->vector_length_expr)
> > + == MATCH_YES))
> > + continue;
> > if ((mask & OMP_CLAUSE_VECTOR)
> > && !c->vector
> > && gfc_match ("vector") == MATCH_YES)
> > @@ -1353,11 +1358,6 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp,
> > uint64_t mask,
> > needs_space = true;
> > continue;
> > }
> > - if ((mask & OMP_CLAUSE_VECTOR_LENGTH)
> > - && c->vector_length_expr == NULL
> > - && (gfc_match ("vector_length ( %e )", &c->vector_length_expr)
> > - == MATCH_YES))
> > - continue;
> > break;
The patch is ok for trunk, but please add there a short comment that
/* vector_length has to be matched before vector, because the latter
doesn't unconditionally match '('. */
>
> Unless there's a more generic problem here (per my comment above), this
> at least needs a source code comment added why "vector_length" needs to
> be handled before "vector".
>
> > --- /dev/null
> > +++ b/gcc/testsuite/gfortran.dg/goacc/vector_length.f90
> > @@ -0,0 +1,11 @@
> > +program t
> > + implicit none
> > + integer, parameter :: n = 100
> > + integer a(n), i
> > +
> > + !$acc parallel loop num_gangs(100) num_workers(1) vector_length(32)
> > + do i = 1, n
> > + a(i) = i
> > + enddo
> > + !$acc end parallel loop
> > +end program t
>
> My preference is that such tests are added to existing test files, for
> example where "!$acc parallel loop" is tested, instead of adding such
> mini files.
No, we want to have as little churn as possible in existing tests, the
general policy is to add new tests (not just for OpenACC/OpenMP, but for
all functionality).
Jakub
