Re: [PATCH] OpenACC: Stand-alone attach/detach clause fixes for Fortran [PR109622]

2023-04-28 Thread Thomas Schwinge
Hi Julian!

On 2023-04-27T11:36:47-0700, Julian Brown  wrote:
> This patch fixes several cases where multiple attach or detach mapping
> nodes were being created for stand-alone attach or detach clauses
> in Fortran.  After the introduction of stricter checking later during
> compilation, these extra nodes could cause ICEs, as seen in the PR.
>
> The patch also fixes cases that "happened to work" previously where
> the user attaches/detaches a pointer to array using a descriptor, and
> (I think!) the "_data" field has offset zero, hence the same address as
> the descriptor as a whole.

Thanks for looking into this.

I haven't reviewed the patch itself, but noticed one thing:

> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.fortran/pr109622-2.f90

> +!$acc enter data copyin(var)

> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.fortran/pr109622-3.f90

> +!$acc enter data copyin(var, tgt)

> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.fortran/pr109622.f90

> +!$acc enter data copyin(var, var2)

You'll want to move these into 'libgomp/testsuite/libgomp.oacc-fortran/'
to actually test them with '-fopenacc' instead of '-fopenmp'.  ;-)


Chalk up one for the idea that I once had, to have '-fopenacc',
'-fopenmp', '-fopenmp-simd' enable '-Wunknown-pragmas' by default.


Grüße
 Thomas
-
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


Re: [PATCH] OpenACC: Stand-alone attach/detach clause fixes for Fortran [PR109622]

2023-04-28 Thread Tobias Burnus

On 27.04.23 20:36, Julian Brown wrote:

This patch fixes several cases where multiple attach or detach mapping
nodes were being created for stand-alone attach or detach clauses
in Fortran.  After the introduction of stricter checking later during
compilation, these extra nodes could cause ICEs, as seen in the PR.

The patch also fixes cases that "happened to work" previously where
the user attaches/detaches a pointer to array using a descriptor, and
(I think!) the "_data" field has offset zero, hence the same address as
the descriptor as a whole.

Tested with offloading to nvptx. OK?


LGTM for mainline and, after some grace period, for GCC 13.

Thanks,

Tobias


2023-04-27  Julian Brown  

  PR fortran/109622

gcc/fortran/
  * trans-openmp.cc (gfc_trans_omp_clauses): Attach/detach clause fixes.

gcc/testsuite/
  * gfortran.dg/goacc/attach-descriptor.f90: Adjust expected output.

libgomp/
  * testsuite/libgomp.fortran/pr109622.f90: New test.
  * testsuite/libgomp.fortran/pr109622-2.f90: New test.
  * testsuite/libgomp.fortran/pr109622-3.f90: New test.

-
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


[PATCH] OpenACC: Stand-alone attach/detach clause fixes for Fortran [PR109622]

2023-04-27 Thread Julian Brown
This patch fixes several cases where multiple attach or detach mapping
nodes were being created for stand-alone attach or detach clauses
in Fortran.  After the introduction of stricter checking later during
compilation, these extra nodes could cause ICEs, as seen in the PR.

The patch also fixes cases that "happened to work" previously where
the user attaches/detaches a pointer to array using a descriptor, and
(I think!) the "_data" field has offset zero, hence the same address as
the descriptor as a whole.

Tested with offloading to nvptx. OK?

Thanks,

Julian

2023-04-27  Julian Brown  

PR fortran/109622

gcc/fortran/
* trans-openmp.cc (gfc_trans_omp_clauses): Attach/detach clause fixes.

gcc/testsuite/
* gfortran.dg/goacc/attach-descriptor.f90: Adjust expected output.

libgomp/
* testsuite/libgomp.fortran/pr109622.f90: New test.
* testsuite/libgomp.fortran/pr109622-2.f90: New test.
* testsuite/libgomp.fortran/pr109622-3.f90: New test.
---
 gcc/fortran/trans-openmp.cc   | 36 +--
 .../gfortran.dg/goacc/attach-descriptor.f90   | 12 +++
 .../testsuite/libgomp.fortran/pr109622-2.f90  | 32 +
 .../testsuite/libgomp.fortran/pr109622-3.f90  | 32 +
 .../testsuite/libgomp.fortran/pr109622.f90| 32 +
 5 files changed, 135 insertions(+), 9 deletions(-)
 create mode 100644 libgomp/testsuite/libgomp.fortran/pr109622-2.f90
 create mode 100644 libgomp/testsuite/libgomp.fortran/pr109622-3.f90
 create mode 100644 libgomp/testsuite/libgomp.fortran/pr109622.f90

diff --git a/gcc/fortran/trans-openmp.cc b/gcc/fortran/trans-openmp.cc
index 4ff9c59df5cb..dbb4a335ab57 100644
--- a/gcc/fortran/trans-openmp.cc
+++ b/gcc/fortran/trans-openmp.cc
@@ -3388,6 +3388,17 @@ gfc_trans_omp_clauses (stmtblock_t *block, 
gfc_omp_clauses *clauses,
  gfc_add_block_to_block (block, );
  if (pointer || allocatable)
{
+ /* If it's a bare attach/detach clause, we just want
+to perform a single attach/detach operation, of the
+pointer itself, not of the pointed-to object.  */
+ if (openacc
+ && (n->u.map_op == OMP_MAP_ATTACH
+ || n->u.map_op == OMP_MAP_DETACH))
+   {
+ OMP_CLAUSE_SIZE (node) = size_zero_node;
+ goto finalize_map_clause;
+   }
+
  node2 = build_omp_clause (input_location,
OMP_CLAUSE_MAP);
  gomp_map_kind kind
@@ -3458,6 +3469,19 @@ gfc_trans_omp_clauses (stmtblock_t *block, 
gfc_omp_clauses *clauses,
{
  if (pointer || (openacc && allocatable))
{
+ /* If it's a bare attach/detach clause, we just want
+to perform a single attach/detach operation, of the
+pointer itself, not of the pointed-to object.  */
+ if (openacc
+ && (n->u.map_op == OMP_MAP_ATTACH
+ || n->u.map_op == OMP_MAP_DETACH))
+   {
+ OMP_CLAUSE_DECL (node)
+   = build_fold_addr_expr (inner);
+ OMP_CLAUSE_SIZE (node) = size_zero_node;
+ goto finalize_map_clause;
+   }
+
  tree data, size;
 
  if (lastref->u.c.component->ts.type == BT_CLASS)
@@ -3494,12 +3518,18 @@ gfc_trans_omp_clauses (stmtblock_t *block, 
gfc_omp_clauses *clauses,
  else if (lastref->type == REF_ARRAY
   && lastref->u.ar.type == AR_FULL)
{
- /* Just pass the (auto-dereferenced) decl through for
-bare attach and detach clauses.  */
+ /* Bare attach and detach clauses don't want any
+additional nodes.  */
  if (n->u.map_op == OMP_MAP_ATTACH
  || n->u.map_op == OMP_MAP_DETACH)
{
- OMP_CLAUSE_DECL (node) = inner;
+ if (GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (inner)))
+   {
+ tree ptr = gfc_conv_descriptor_data_get (inner);
+ OMP_CLAUSE_DECL (node) = ptr;
+   }
+ else
+   OMP_CLAUSE_DECL (node) = inner;
  OMP_CLAUSE_SIZE (node) = size_zero_node;
  goto finalize_map_clause;
}
diff --git