Re: [PATCH, v2, OpenMP 5.2, Fortran] Strictly-structured block support for OpenMP directives

[Jakub Jelinek <jakub@redhat.com>]

On Wed, Oct 20, 2021 at 08:30:34PM +0800, Chung-Lin Tang wrote:
> 2021-10-20  Chung-Lin Tang  <cltang@codesourcery.com>
> 
> gcc/fortran/ChangeLog:
> 
> 	* decl.c (gfc_match_end): Add COMP_OMP_STRICTLY_STRUCTURED_BLOCK case
> 	together with COMP_BLOCK.
> 	* parse.c (parse_omp_structured_block): Change return type to
> 	'gfc_statement', add handling for strictly-structured block case, adjust
> 	recursive calls to parse_omp_structured_block.
> 	(parse_executable): Adjust calls to parse_omp_structured_block.
> 	* parse.h (enum gfc_compile_state): Add
> 	COMP_OMP_STRICTLY_STRUCTURED_BLOCK.
> 	* trans-openmp.c (gfc_trans_omp_workshare): Add EXEC_BLOCK case
> 	handling.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gfortran.dg/gomp/cancel-1.f90: Adjust testcase.
> 	* gfortran.dg/gomp/nesting-3.f90: Adjust testcase.
> 	* gfortran.dg/gomp/strictly-structured-block-1.f90: New test.
> 	* gfortran.dg/gomp/strictly-structured-block-2.f90: New test.
> 	* gfortran.dg/gomp/strictly-structured-block-3.f90: New test.
> 
> libgomp/ChangeLog:
> 
> 	* libgomp.texi (Support of strictly structured blocks in Fortran):
> 	Adjust to 'Y'.
> 	* testsuite/libgomp.fortran/task-reduction-16.f90: Adjust testcase.

Thanks, looks mostly good now, but I still have nits for the testsuite.

> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/gomp/strictly-structured-block-1.f90
> @@ -0,0 +1,211 @@
> +! { dg-do compile }
> +! { dg-options "-fopenmp" }
> +
> +program main
> +  integer :: x, i, n
> +
> +  !$omp parallel
> +  block
> +    x = x + 1
> +  end block

I'd prefer not to use those x = j or x = x + 1 etc.
as statements that do random work here whenever possible.
While those are dg-do compile testcases, especially if
it is without dg-errors I think it is preferrable not to show
bad coding examples.
E.g. the x = x + 1 above is wrong for 2 reasons, x is uninitialized
before the parallel, and there is a data race, the threads, teams etc.
can write to x concurrently.
I think better would be to use something like
    call do_work
which doesn't have to be defined anywhere and will just stand there
as a black box for unspecified work.

> +  !$omp workshare
> +  block
> +    x = x + 1
> +  end block

There are exceptions though, e.g. workshare is such a case, because
e.g. call do_work is not valid in workshare.
So, it is ok to keep using x = x + 1 here if you initialize it
first at the start of the program.

> +  !$omp workshare
> +  block
> +    x = 1
> +    !$omp critical
> +    block
> +      x = 3
> +    end block
> +  end block

And then there are cases like the above, please
just use different variables there (all initialized) or
say an array and access different elements in the different spots.

	Jakub