Re: [PATCH] Fix OpenACC vector_length parsing in fortran

[Jakub Jelinek <jakub at redhat dot com>]

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 <cesar@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