Re: [PATCH] OpenACC: Further attach/detach clause fixes for Fortran [PR109622]

[Tobias Burnus <tobias@codesourcery.com>]

On 03.05.23 14:59, Julian Brown wrote:
> How does this version look?
> Retested with offloading to nvptx.
LGTM (for mainline + GCC 13 backporting) but I have two tiny comments:
> diff --git a/gcc/fortran/openmp.cc b/gcc/fortran/openmp.cc
> index 86e4515..322856a 100644
> --- a/gcc/fortran/openmp.cc
> +++ b/gcc/fortran/openmp.cc
> @@ -7711,6 +7711,23 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses *omp_clauses,
>                                    &n->where);
>                     }
>                 }
> +             if (openacc
> +                 && list == OMP_LIST_MAP
> +                 && (n->u.map_op == OMP_MAP_ATTACH
> +                     || n->u.map_op == OMP_MAP_DETACH))
> +               {
> +                 symbol_attribute attr;
> +                 gfc_clear_attr (&attr);
> +                 if (n->expr)
> +                   attr = gfc_expr_attr (n->expr);
> +                 else if (n->sym)
> +                   attr = n->sym->attr;

Note that n->sym == NULL is only the case if the argument was
omp_all_memory (→ gfc_match_omp_variable_list); that can only be the
case for OMP_CLAUSE_DEPEND.

As OpenMP's 'depend' clause is handled before and you additionally deal
with OpenACC, only, you could just use 'else' instead of 'else if
(n->sym)' – and also get rid of the 'gfc_clear_attr' as the values get
overwritten by the assignment and by the function call.

Later code (e.g. line 7785 in the current code) also assumes (for
OpenACC + MAP) that n->sym != NULL by bluntly dereferencing it.

@@ -3523,15 +3525,20 @@ gfc_trans_omp_clauses (stmtblock_t *block,
gfc_omp_clauses *clauses,
>                     if (n->u.map_op == OMP_MAP_ATTACH
>                         || n->u.map_op == OMP_MAP_DETACH)
>                       {
> -                       if (GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (inner)))
> +                       if (POINTER_TYPE_P (TREE_TYPE (inner))
> +                           || GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (inner)))
>                           {
...
>                           }
> -                       else
> -                         OMP_CLAUSE_DECL (node) = inner;
> -                       OMP_CLAUSE_SIZE (node) = size_zero_node;
> -                       goto finalize_map_clause;
>                       }

You can now combine the two if conditions, which avoids some indenting
and should permit to put 'tree ptr' / ' = ...' again on the same line.

Thanks for the patch!

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