Fix OpenACC vector_length parsing in fortran

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.

Is this OK for trunk and gcc6?

Cesar

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?

> --- 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;

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.


Grüße
 Thomas

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

On Thu, Jul 14, 2016 at 07:05:52PM -0700, Cesar Philippidis wrote:
> Is this OK for trunk and gcc6?

See other mail for trunk approval, just want to comment on the "and gcc6",
it doesn't apply to gcc6, you can add the testcase there,
but gcc 6 matches vector_length before vector, the bug has been introduced
by my gfc_match_omp_clauses alphabetical sorting.

I went through the whole function and this case is the only one that
needs the non-alphabetical match.

	Jakub

Hi!

Including <gcc@gcc.gnu.org> in case others also have opinions on "GCC
testsuite maintenance".

On Fri, 15 Jul 2016 10:21:13 +0200, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Jul 15, 2016 at 09:44:25AM +0200, Thomas Schwinge wrote:
> > Does that me we (erroneously) accept any random junk after [...]
> 
> No.  The thing is, [...]

Thanks for the explanation!


> > On Thu, 14 Jul 2016 19:05:52 -0700, Cesar Philippidis <cesar@codesourcery.com> wrote:
> > > --- /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.

One of the problem I have with this specific and similar other test cases
is, that when coming back to such a test case in a few weeks' time, it is
not obvious at all what this is testing, whereas if this were inside a
file that generally/already tests all kinds of "!$acc parallel loop"
variants, this would be obvious.

> 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).

Hmm, that's something I had not been aware of, and I can't find this
covered in the documentation.  So, you're basically saying that files in
the testsuite are write-once, and should not be maintained aside from
fixing errors, adjusting due to optimization changes, or due to changed
diagnostics, and the like?  (Of course, I do agree that we shouldn't
"randomly" remove/modify test cases that test stuff that is still
relevant, but that doesn't apply here, obviously.)

In my opinion, the testsuite should be maintained, just like "regular"
code, and so related test cases should be consolidated, obsolete ones be
removed, and so on, every once in a while, or on demand.  So that the GCC
testsuite doesn't just always grow (in number of files), but also "stays
relevant".  Also, the wall-time overhead is much lower if, taking this
specific example, that one OpenACC directive test case is tested next to
others in an existing testsuite file, as opposed to requiring a separate
compiler invocation, being in a separate file.


Grüße
 Thomas

On Fri, 15 Jul 2016, Thomas Schwinge wrote:

> > 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).
> 
> Hmm, that's something I had not been aware of, and I can't find this
> covered in the documentation.  So, you're basically saying that files in
> the testsuite are write-once, and should not be maintained aside from
> fixing errors, adjusting due to optimization changes, or due to changed
> diagnostics, and the like?  (Of course, I do agree that we shouldn't

Yes, that's my view.  It makes it easier to distinguish regressions from 
new tests that fail (on some platforms) if what a test tests doesn't 
change unnecessarily.

On Jul 25, 2016, at 9:37 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> 
> On Fri, 15 Jul 2016, Thomas Schwinge wrote:
> 
>>> 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).
>> 
>> Hmm, that's something I had not been aware of, and I can't find this
>> covered in the documentation.  So, you're basically saying that files in
>> the testsuite are write-once, and should not be maintained aside from
>> fixing errors, adjusting due to optimization changes, or due to changed
>> diagnostics, and the like?  (Of course, I do agree that we shouldn't
> 
> Yes, that's my view.  It makes it easier to distinguish regressions from 
> new tests that fail (on some platforms) if what a test tests doesn't 
> change unnecessarily.

Right.  Roughly the test suite is a monotonic, slow moving ever increasing mass.  Generally we favor new files for new tests and little to no recoding or rearrangement of existing stuff.  We do some limited forms for maintenance, for example, intptr_t to make a less portable test case, to be more portable, just to pick a random recent edit; but these tend to be very minor and very narrowly focused.