Re: [PATCH 10/13] OpenACC 2.6 deep copy: Fortran front-end parts

[Tobias Burnus <tobias@codesourcery.com>]

On 12/18/19 7:04 AM, Julian Brown wrote:

> This part contains the Fortran front-end support for parsing the new
> attach and detach clauses, as well as derived-type members on other
> data-movement clauses (copyin, copyout, etc.).
I browsed the patch and it looks mostly fine to me. However, I do have 
comments related to the array refs.
> @@ -3890,9 +3922,6 @@ check_array_not_assumed (gfc_symbol *sym, locus loc, const char *name)
>   static void
>   resolve_oacc_data_clauses (gfc_symbol *sym, locus loc, const char *name)
>   {
> +	  /* Disallow duplicate bare variable references and multiple
> +	     subarrays of the same array here, but allow multiple components of
> +	     the same (e.g. derived-type) variable.  For the latter, duplicate
> +	     components are detected elsewhere.  */
Do we have a test case for "the latter"?
> @@ -4470,23 +4514,43 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses *omp_clauses,
> +		gfc_ref *array_ref = NULL;
> +		bool resolved = false;
>   		if (n->expr)
>   		  {
> -		    if (!gfc_resolve_expr (n->expr)
> +		    array_ref = n->expr->ref;
> +		    resolved = gfc_resolve_expr (n->expr);
> +
> +		    /* Look through component refs to find last array
> +		       reference.  */
> +		    if (openacc)
> +		      while (resolved
I would move the "resolved" into the "if" condition as it doesn't change 
in the while loop.
> +			     && array_ref
> +			     && (array_ref->type == REF_COMPONENT
> +				 || (array_ref->type == REF_ARRAY
> +				     && array_ref->next
> +				     && (array_ref->next->type
> +					 == REF_COMPONENT))))
> +			array_ref = array_ref->next;
> +		  }
> +		if (array_ref
> +		    || (n->expr
> +			&& (!resolved || n->expr->expr_type != EXPR_VARIABLE)))
> +		  {
> +		    if (!resolved
>   			|| n->expr->expr_type != EXPR_VARIABLE
> -			|| n->expr->ref == NULL
> -			|| n->expr->ref->next
> -			|| n->expr->ref->type != REF_ARRAY)
> +			|| array_ref->next
> +			|| array_ref->type != REF_ARRAY)
>   		      gfc_error ("%qs in %s clause at %L is not a proper "
>   				 "array section", n->sym->name, name,
>   				 &n->where);
> -		    else if (n->expr->ref->u.ar.codimen)
> +		    else if (array_ref->u.ar.codimen)
>   		      gfc_error ("Coarrays not supported in %s clause at %L",
>   				 name, &n->where);

First, I believe the error message is wrong – coarrays are permitted but 
only if their local data is accessed; this check checks whether a 
coindex is present, i.e. whether the variable is accessed on a remote 
process ("image"). Hence, the error should use something like "Entry 
shall not be coindexed in %s clause at %L" or something like that.

Secondly, a coarray can exist at different places, e.g.

type t
   integer :: i
end type t
type t2
   integer, allocatable :: i[:]
   type(t), allocatable :: x[:]
end type t2
type(t), allocatable :: A[:], B[:]
type(t) :: D[*]
type(t2) :: C
!$acc data copy(D[2]%i, A[4], B[4]%i, C%i[2], C%x[4]%i)

Here, C is not a coarray. But all those list items in the clause are
coindexed – but your new check will only detect those where the ultimate
component is coindexed. The quickest check for this is "gfc_is_coindexed (expr)".

Thirdly, I am not sure whether the following will work with your code:
type t
   integer :: i(5), j(17), k
end type t
type(t) :: x(10)
!$acc data copy (x(:)%k, x(:)%j(3))

This data is strided; I don't quickly see whether that's rejected. (I also
didn't check whether it is valid, but I think it is not.)

> +/* Transparently dereference VAR if it is a pointer, reference, etc.
> +   according to Fortran semantics.  */
> +
> +tree
> +gfc_auto_dereference_var (gfc_symbol *sym, tree var, bool descriptor_only_p,
> +			  bool is_classarray)

I have to admit that 'transparently deference' and 'auto' puzzles me, 
naming/description wise, but I don't have a good solution; but I like 
the 'Dereference the expression, where needed' more

> +      /* Dereference the expression, where needed.  */
> +      se->expr = gfc_auto_dereference_var (sym, se->expr, se->descriptor_only,
> +					   is_classarray);

> +++ b/gcc/fortran/trans-openmp.c
> +  if (element)
> +    {
> +      gfc_conv_expr_reference (&se, n->expr);
> +      gfc_add_block_to_block (block, &se.pre);
> +      ptr = se.expr;
> +      OMP_CLAUSE_SIZE (node)
> +	= TYPE_SIZE_UNIT (TREE_TYPE (ptr));

This fits nicely on a single line; I think the ';' is in column 64!

Cheers,

Tobias