Re: [Patch] OpenMP/Fortran: Permit end-clause on directive

[Jakub Jelinek <jakub@redhat.com>]

On Fri, Aug 26, 2022 at 08:21:26PM +0200, Tobias Burnus wrote:
> I did run into some issues related to this; those turned out to be
> unrelated, but I end ended up implementing this feature.
> 
> Side remark: 'omp parallel workshare' seems to actually permit 'nowait'
> now, but I guess that's an unintended change due to the
> syntax-representation change. Hence, it is now tracked as Spec Issue
> 3338 and I do not permit it.
> 
> OK for mainline?
> 
> Tobias
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

> OpenMP/Fortran: Permit end-clause on directive
> 
> gcc/fortran/ChangeLog:
> 
> 	* openmp.cc (OMP_DO_CLAUSES, OMP_SCOPE_CLAUSES,
> 	OMP_SECTIONS_CLAUSES, OMP_SINGLE_CLAUSES): Add 'nowait'.

This doesn't describe what the patch actually does, Add 'nowait'.
is only true for the first 3, for OMP_SINGLE_CLAUSES IMHO you
want a separate
	(OMP_SINGLE_CLAUSES): Add 'nowait' and 'copyprivate'.
entry.

> @@ -3855,7 +3857,7 @@ cleanup:
>     | OMP_CLAUSE_ORDER | OMP_CLAUSE_ALLOCATE)
>  #define OMP_SINGLE_CLAUSES \
>    (omp_mask (OMP_CLAUSE_PRIVATE) | OMP_CLAUSE_FIRSTPRIVATE		\
> -   | OMP_CLAUSE_ALLOCATE)
> +   | OMP_CLAUSE_ALLOCATE | OMP_CLAUSE_NOWAIT | OMP_CLAUSE_COPYPRIVATE)
>  #define OMP_ORDERED_CLAUSES \
>    (omp_mask (OMP_CLAUSE_THREADS) | OMP_CLAUSE_SIMD)
>  #define OMP_DECLARE_TARGET_CLAUSES \

> @@ -5909,13 +5915,11 @@ gfc_match_omp_teams_distribute_simd (void)
>  match
>  gfc_match_omp_workshare (void)
>  {
> -  if (gfc_match_omp_eos () != MATCH_YES)
> -    {
> -      gfc_error ("Unexpected junk after $OMP WORKSHARE statement at %C");
> -      return MATCH_ERROR;
> -    }
> +  gfc_omp_clauses *c;
> +  if (gfc_match_omp_clauses (&c, omp_mask (OMP_CLAUSE_NOWAIT)) != MATCH_YES)
> +    return MATCH_ERROR;
>    new_st.op = EXEC_OMP_WORKSHARE;
> -  new_st.ext.omp_clauses = gfc_get_omp_clauses ();
> +  new_st.ext.omp_clauses = c;
>    return MATCH_YES;
>  }

I think it would be better to introduce OMP_WORKSHARE_CLAUSES and use
it in both gfc_match_omp_workshare and just use
  return match_omp (EXEC_OMP_WORKSHARE, OMP_WORKSHARE_CLAUSES);
?

> @@ -6954,6 +6952,9 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses *omp_clauses,
>  	      }
>  	    break;
>  	  case OMP_LIST_COPYPRIVATE:
> +	    if (omp_clauses->nowait)
> +	      gfc_error ("NOWAIT clause must not be be used with COPYPRIVATE "

s/be be/be/
> +			 "clause at %L", &n->where);
>  	    for (; n != NULL; n = n->next)
>  	      {
>  		if (n->sym->as && n->sym->as->type == AS_ASSUMED_SIZE)

> @@ -5284,7 +5285,13 @@ parse_omp_do (gfc_statement omp_st)
>    if (st == omp_end_st)
>      {
>        if (new_st.op == EXEC_OMP_END_NOWAIT)
> -	cp->ext.omp_clauses->nowait |= new_st.ext.omp_bool;
> +	{
> +	  if (cp->ext.omp_clauses->nowait && new_st.ext.omp_bool)
> +	    gfc_error_now ("Duplicated NOWAIT clause on %s and %s at %C",
> +			   gfc_ascii_statement (omp_st),
> +			   gfc_ascii_statement (omp_end_st));
> +	  cp->ext.omp_clauses->nowait |= new_st.ext.omp_bool;
> +	}
>        else
>  	gcc_assert (new_st.op == EXEC_NOP);
>        gfc_clear_new_st ();

Not sure if the standard is clear enough that unique clauses can't be
repeated on both directive and corresponding end directive.  But let's
assume that is the case.

> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/gomp/copyprivate-2.f90
> @@ -0,0 +1,69 @@
> +  FUNCTION t()
> +    INTEGER :: a, b, t
> +    a = 0
> +    t = b
> +    b = 0
> +    !$OMP PARALLEL REDUCTION(+:b)
> +      !$OMP SINGLE COPYPRIVATE (b) NOWAIT  ! { dg-error "NOWAIT clause must not be be used with COPYPRIVATE clause" }

Here too (several times).

> +        !$OMP ATOMIC WRITE
> +        b = 6
> +      !$OMP END SINGLE
> +    !$OMP END PARALLEL
> +    t = t + b
> +  END FUNCTION
> +
> +  FUNCTION t2()
> +    INTEGER :: a, b, t2
> +    a = 0
> +    t2 = b
> +    b = 0
> +    !$OMP PARALLEL REDUCTION(+:b)
> +      !$OMP SINGLE NOWAIT COPYPRIVATE (b)  ! { dg-error "NOWAIT clause must not be be used with COPYPRIVATE clause" }
> +        !$OMP ATOMIC WRITE
> +        b = 6
> +      !$OMP END SINGLE
> +    !$OMP END PARALLEL
> +    t2 = t2 + b
> +  END FUNCTION
> +
> +  FUNCTION t3()
> +    INTEGER :: a, b, t3
> +    a = 0
> +    t3 = b
> +    b = 0
> +    !$OMP PARALLEL REDUCTION(+:b)
> +      !$OMP SINGLE COPYPRIVATE (b)  ! { dg-error "NOWAIT clause must not be be used with COPYPRIVATE clause" }
> +        !$OMP ATOMIC WRITE
> +        b = 6
> +      !$OMP END SINGLE NOWAIT
> +    !$OMP END PARALLEL
> +    t3 = t3 + b
> +  END FUNCTION
> +
> +  FUNCTION t4()
> +    INTEGER :: a, b, t4
> +    a = 0
> +    t4 = b
> +    b = 0
> +    !$OMP PARALLEL REDUCTION(+:b)
> +      !$OMP SINGLE
> +        !$OMP ATOMIC WRITE
> +        b = 6
> +      !$OMP END SINGLE NOWAIT COPYPRIVATE (b)  ! { dg-error "NOWAIT clause must not be be used with COPYPRIVATE clause" }
> +    !$OMP END PARALLEL
> +    t4 = t4 + b
> +  END FUNCTION
> +
> +  FUNCTION t5()
> +    INTEGER :: a, b, t5
> +    a = 0
> +    t5 = b
> +    b = 0
> +    !$OMP PARALLEL REDUCTION(+:b)
> +      !$OMP SINGLE
> +        !$OMP ATOMIC WRITE
> +        b = 6
> +      !$OMP END SINGLE COPYPRIVATE (b) NOWAIT  ! { dg-error "NOWAIT clause must not be be used with COPYPRIVATE clause" }
> +    !$OMP END PARALLEL
> +    t5 = t5 + b
> +  END FUNCTION

I think this lacks a test for !$OMP SINGLE NOWAIT and !$OMP END SINGLE COPYPRIVATE (b).

Also, shouldn't we have test coverage for !$OMP SINGLE COPYPRIVATE (b) with !$OMP END SINGLE COPYPRIVATE (b)
(that we detect multiple copyprivate clauses for the same variable even that
way)?

Otherwise LGTM.

Note, for combined constructs with target seems we were already implementing
this, because the 5.1 wording allows nowait only on !$omp target and not on
!$omp end target, right?

	Jakub