Re: [Patch] Fortran/OpenMP: Add omp loop [PR99928]

[Jakub Jelinek <jakub@redhat.com>]

On Thu, Jun 03, 2021 at 12:30:50AM +0200, Tobias Burnus wrote:
> This patch adds support for 'omp loop' to gfortran including the combined
> constructs. It also fixes some splitting issues with clauses in
> combined constructs.
> 
> It does not attempt to clean up all remaining Fortran issues with
> clauses in combined constructs (cf. below + PR).
> 
>  * * *
> 
> Since 'parallel loop' is now supported, using
>  !$omp parallel &
>  !$acc& loop
> no longer gave an error. (Same result before: '!$acc& do'.)

I think best would be just to remove that part of the testcase
in the loop patch and handle the !$omp with !$acc continuations
and vice versa in a separate change.  That seems to be a preexisting
bug not really related to whether we support loop or not.
fatal_error is certainly not ideal, but I can understand fixing
it otherwise might be hard.
Wonder if we just shouldn't treat the incorrect continuations
as if they were simple comments.

> The check does (for Fortran – and alike for C):
>   ! { dg-final { scan-tree-dump-not "omp distribute\[^\n\r]*lastprivate\\(j00\\)" "gimple" } }
>   ! { dg-final { scan-tree-dump-not "omp parallel\[^\n\r]*lastprivate\\(j00\\)" "gimple" } }
>   ! { dg-final { scan-tree-dump-not "omp for\[^\n\r]*lastprivate\\(j00\\)" "gimple" } }
>   ! { dg-final { scan-tree-dump "omp simd\[^\n\r]*linear\\(j00:1\\)" "gimple" } }
>   !$omp distribute parallel do simd default(none)
>   do j00 = 1, 64
>   end do
> 
> While C generates (original dump)
> 
>   #pragma omp distribute
>       #pragma omp parallel default(none)
>             #pragma omp for nowait
>                 #pragma omp simd
> 
> Fortran generates the same except for:
>                     #pragma omp simd linear(j00:1)

The *-7 testcase is not appropriate for Fortran, it tests the
IV declared in the construct cases which Fortran has no counterpart for,
even if the IV is first seen within the construct, that would implicitly
declare it outside of the construct.

> +	      if (gfc_match ("teams )") == MATCH_YES)
> +		c->bind = OMP_BIND_TEAMS;
> +	      else if (gfc_match ("parallel )") == MATCH_YES)
> +		c->bind = OMP_BIND_PARALLEL;
> +	      else if (gfc_match ("thread )") == MATCH_YES)
> +		c->bind = OMP_BIND_THREAD;
> +	      else
> +		{
> +		  gfc_error ("Expected TEAMS, PARALLEL or THEAD as binding in "

Typo, s/THEAD/THREAD/

> +      if (clauses->bind == OMP_BIND_TEAMS)
> +        OMP_CLAUSE_BIND_KIND (c) = OMP_CLAUSE_BIND_TEAMS;
> +      else if (clauses->bind == OMP_BIND_PARALLEL)
> +        OMP_CLAUSE_BIND_KIND (c) = OMP_CLAUSE_BIND_PARALLEL;
> +      else if (clauses->bind == OMP_BIND_THREAD)
> +        OMP_CLAUSE_BIND_KIND (c) = OMP_CLAUSE_BIND_THREAD;
> +      else
> +	gcc_unreachable ();

Wouldn't a switch (clauses->bind) be cleaner?

Otherwise LGTM.

	Jakub