Re: [Patch] OpenMP/Fortran: Add support for enter clause on declare target (was: [committed] openmp: Add support for enter clause on declare target)

[Jakub Jelinek <jakub@redhat.com>]

On Fri, May 27, 2022 at 04:52:17PM +0200, Tobias Burnus wrote:
> This patch adds the Fortran support to the just-committed C/C++ support for the 'enter' clause.
> 
> The 'TO'/'ENTER' usage is first stored in a linked list – and
> then as attribute to the symbol. I am not sure how to handle it best.
> I did went for adding an ENTER_LIST but kept only using the single attribute.
> 
> Result: In one diagnostic, I use 'TO or ENTER clause'  in the other,
> I can properly use 'TO' or 'ENTER' clause.
> 
> This could be kept as is, but to save memory it would be possible
> to drop the ENTER_LIST – or, to improve diagnostic, we could try harder
> (e.g. by re-walking the list or by another gfc_attribute). Preferences?
> If not, I would go for the attached middle way.
> 
> 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: Add support for enter clause on declare target
> 
> Fortran version to C/C++ commit r13-797-g0ccba4ed8571c18c7015413441e971
> 
> gcc/fortran/ChangeLog:
> 
> 	* dump-parse-tree.cc (show_omp_clauses): Handle OMP_LIST_ENTER.
> 	* gfortran.h: Add OMP_LIST_ENTER.
> 	* openmp.cc (enum omp_mask2, OMP_DECLARE_TARGET_CLAUSES): Add
> 	OMP_CLAUSE_ENTER.
> 	(gfc_match_omp_clauses, gfc_match_omp_declare_target,
> 	resolve_omp_clauses): Handle 'enter' clause.
> 
> libgomp/ChangeLog:
> 
> 	* libgomp.texi (OpenMP 5.2): Mark 'enter' clause as supported.
> 	* testsuite/libgomp.fortran/declare-target-1.f90: Extend to test
> 	explicit 'to' and 'enter' clause.
> 	* testsuite/libgomp.fortran/declare-target-2.f90: Update accordingly.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gfortran.dg/gomp/declare-target-2.f90: Add 'enter' clause test.
> 	* gfortran.dg/gomp/declare-target-4.f90: Likewise.

Mostly good, but see below.

> -  for (list = OMP_LIST_TO; list != OMP_LIST_NUM;
> -       list = (list == OMP_LIST_TO ? OMP_LIST_LINK : OMP_LIST_NUM))
> +  for (list = OMP_LIST_ENTER; list != OMP_LIST_NUM;
> +       list = (list == OMP_LIST_ENTER
> +	       ? OMP_LIST_TO
> +	       : (list == OMP_LIST_TO ? OMP_LIST_LINK : OMP_LIST_NUM)))
>      for (n = c->lists[list]; n; n = n->next)
>        if (n->sym)
>  	{
> @@ -4564,14 +4584,14 @@ gfc_match_omp_declare_target (void)
>  		   && n->sym->attr.omp_declare_target_link
>  		   && list != OMP_LIST_LINK)
>  	    gfc_error_now ("OMP DECLARE TARGET variable at %L previously "
> -			   "mentioned in LINK clause and later in TO clause",
> -			   &n->where);
> +			   "mentioned in LINK clause and later in %s clause",
> +			   &n->where, list == OMP_LIST_TO ? "TO" : "ENTER");
>  	  else if (n->sym->attr.omp_declare_target
>  		   && !n->sym->attr.omp_declare_target_link
>  		   && list == OMP_LIST_LINK)
>  	    gfc_error_now ("OMP DECLARE TARGET variable at %L previously "
> -			   "mentioned in TO clause and later in LINK clause",
> -			   &n->where);
> +			   "mentioned in TO or ENTER clause and later in "
> +			   "LINK clause", &n->where);

The wording of the messages "previously mentioned in FOO and later in BAR clause"
makes not much sense to me, because we in the Fortran FE don't remember which
clause was specified earlier and which one was later.
The loop is walking first all enter clauses, then all to clauses, then all
link clauses.

Now, if we change the wording so that it complains that a variable is
"mentioned in both FOO and BAR clauses", we could avoid the TO or ENTER
stuff even without some O(n^2) complexity or extra bit somewhere simply
by walking the clauses in the order of TO, LINK, ENTER (or ENTER, LINK, TO)
clauses, wer could be exact.

Though, further thinking about it, this isn't just about the case
where it is on the same declare target, but also on multiple and in that
case previous/later makes sense.

As we don't remember if it was previously TO or ENTER, I'd just suggest:
1) simplify the 2 for cycles, with 3 lists it is too unreadable, so use
   something like:
  static const int to_enter_link_lists[]
    = { OMP_LIST_TO, OMP_LIST_ENTER, OMP_LIST_LINK };
  for (int listn = 0; listn < ARRAY_SIZE (to_enter_link_lists)
		      && (list = to_enter_link_lists[listn], true); ++listn)
2) move the
          else if (n->sym->mark)
            gfc_error_now ("Variable at %L mentioned multiple times in "
                           "clauses of the same OMP DECLARE TARGET directive",
                           &n->where);
  diagnostics earlier, above
          else if (n->sym->attr.omp_declare_target
                   && n->sym->attr.omp_declare_target_link
                   && list != OMP_LIST_LINK)
  and adjust testsuite if needed

Ok with those changes.

On the C/C++ FE side, I'll need to change %<to%> to %<to%> or %<enter%>.

	Jakub