Re: [PATCH] OpenACC: Further attach/detach clause fixes for Fortran [PR109622]
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,
>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 ();
+ 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
Re: [PATCH] OpenACC: Further attach/detach clause fixes for Fortran [PR109622]
On Tue, 2 May 2023 12:29:22 +0200
Tobias Burnus wrote:
> On 29.04.23 12:57, Julian Brown wrote:
> > This patch moves several tests introduced by the following patch:
> >
> >https://gcc.gnu.org/pipermail/gcc-patches/2023-April/616939.html
> >
>
> I believe you intent this as git log entry. Can you add
>... commit r14-325-gcacf65d74463600815773255e8b82b4043432bd7
> as this makes looking at the git history easier.
Added.
> > into the proper location for OpenACC testing (thanks to Thomas for
> > spotting my mistake!), and also fixes a few additional problems --
> > missing diagnostics for non-pointer attaches, and a case where a
> > pointer was incorrectly dereferenced. Tests are also adjusted for
> > vector-length warnings on nvidia accelerators.
> >
> > Tested with offloading to nvptx. OK?
> >
> > 2023-04-29 Julian Brown
> >
> > PR fortran/109622
> >
> > gcc/fortran/
> > * trans-openmp.cc (gfc_trans_omp_clauses): Add diagnostic for
> > non-pointer/non-allocatable attach/detach. Remove
> > dereference for pointer-to-scalar derived type component
> > attach/detach.
>
> In general, we prefer resolution-time diagnostic to tree-translation
> diagnostic, unless there is a good reason to do the latter.
> At a glance, it should be even sufficient to have a single diagnostic
> instead of two when placed into openmp.cc.
How does this version look?
Retested with offloading to nvptx.
Thanks,
Julian
commit 43be8cd7a3e86af421e611c72f714b6c40f35bba
Author: Julian Brown
Date: Fri Apr 28 22:27:54 2023 +
OpenACC: Further attach/detach clause fixes for Fortran [PR109622]
This patch moves several tests introduced by the following patch:
https://gcc.gnu.org/pipermail/gcc-patches/2023-April/616939.html
commit r14-325-gcacf65d74463600815773255e8b82b4043432bd7
into the proper location for OpenACC testing (thanks to Thomas for
spotting my mistake!), and also fixes a few additional problems --
missing diagnostics for non-pointer attaches, and a case where a pointer
was incorrectly dereferenced. Tests are also adjusted for vector-length
warnings on nvidia accelerators.
2023-04-29 Julian Brown
PR fortran/109622
gcc/fortran/
* openmp.cc (resolve_omp_clauses): Add diagnostic for
non-pointer/non-allocatable attach/detach.
* trans-openmp.cc (gfc_trans_omp_clauses): Remove dereference for
pointer-to-scalar derived type component attach/detach. Fix
attach/detach handling for descriptors.
gcc/testsuite/
* gfortran.dg/goacc/pr109622-5.f90: New test.
* gfortran.dg/goacc/pr109622-6.f90: New test.
libgomp/
* testsuite/libgomp.fortran/pr109622.f90: Move test...
* testsuite/libgomp.oacc-fortran/pr109622.f90: ...to here. Ignore
vector length warning.
* testsuite/libgomp.fortran/pr109622-2.f90: Move test...
* testsuite/libgomp.oacc-fortran/pr109622-2.f90: ...to here. Add
missing copyin/copyout variable. Ignore vector length warnings.
* testsuite/libgomp.fortran/pr109622-3.f90: Move test...
* testsuite/libgomp.oacc-fortran/pr109622-3.f90: ...to here. Ignore
vector length warnings.
* testsuite/libgomp.oacc-fortran/pr109622-4.f90: New test.
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,
>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 ();
+ if (n->expr)
+ attr = gfc_expr_attr (n->expr);
+ else if (n->sym)
+ attr = n->sym->attr;
+ if (!attr.pointer && !attr.allocatable)
+ gfc_error ("%qs clause argument must be ALLOCATABLE or "
+ "a POINTER at %L",
+ (n->u.map_op == OMP_MAP_ATTACH) ? "attach"
+ : "detach", >where);
+ }
if (lastref
|| (n->expr
&& (!resolved || n->expr->expr_type != EXPR_VARIABLE)))
diff --git a/gcc/fortran/trans-openmp.cc b/gcc/fortran/trans-openmp.cc
index 6ee22fa..e5de85b 100644
--- a/gcc/fortran/trans-openmp.cc
+++ b/gcc/fortran/trans-openmp.cc
@@ -3395,6 +3395,8 @@ gfc_trans_omp_clauses (stmtblock_t *block, gfc_omp_clauses *clauses,
&& (n->u.map_op == OMP_MAP_ATTACH
|| n->u.map_op == OMP_MAP_DETACH))
{
+ OMP_CLAUSE_DECL (node)
+ = build_fold_addr_expr (OMP_CLAUSE_DECL (node));
OMP_CLAUSE_SIZE (node) = size_zero_node;
goto finalize_map_clause;
Re: [PATCH] OpenACC: Further attach/detach clause fixes for Fortran [PR109622]
Hi Julian!
On 2023-04-29T03:57:41-0700, Julian Brown wrote:
> This patch moves several tests introduced by the following patch:
>
> https://gcc.gnu.org/pipermail/gcc-patches/2023-April/616939.html
>
> into the proper location for OpenACC testing (thanks to Thomas for
> spotting my mistake!), and also fixes a few additional problems --
> missing diagnostics for non-pointer attaches, and a case where a pointer
> was incorrectly dereferenced. Tests are also adjusted for vector-length
> warnings on nvidia accelerators.
>
> Tested with offloading to nvptx. OK?
Thanks for looking into this.
I haven't reviewed the patch itself, but noticed one thing:
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/goacc/pr109622-5.f90
> @@ -0,0 +1,45 @@
> +! { dg-do compile }
> +
> +use openacc
[...]/gfortran.dg/goacc/pr109622-5.f90:3:5: Fatal Error: Cannot open module
file 'openacc.mod' for reading at (1): No such file or directory
... for GCC build-tree testing. Just remove the 'use openacc'; it's not
necessary here.
Grüße
Thomas
> +implicit none
> +
> +type t
> +integer :: foo
> +character(len=8) :: bar
> +integer :: qux(5)
> +end type t
> +
> +type(t) :: var
> +
> +var%foo = 3
> +var%bar = "HELLOOMP"
> +var%qux = (/ 1, 2, 3, 4, 5 /)
> +
> +!$acc enter data copyin(var)
> +
> +!$acc enter data attach(var%foo)
> +! { dg-error "'attach' clause argument not pointer or allocatable" "" {
> target *-*-* } .-1 }
> +!$acc enter data attach(var%bar)
> +! { dg-error "'attach' clause argument not pointer or allocatable" "" {
> target *-*-* } .-1 }
> +!$acc enter data attach(var%qux)
> +! { dg-error "'attach' clause argument not pointer or allocatable" "" {
> target *-*-* } .-1 }
> +
> +!$acc serial
> +var%foo = 5
> +var%bar = "GOODBYE!"
> +var%qux = (/ 6, 7, 8, 9, 10 /)
> +!$acc end serial
> +
> +!$acc exit data detach(var%qux)
> +! { dg-error "'detach' clause argument not pointer or allocatable" "" {
> target *-*-* } .-1 }
> +!$acc exit data detach(var%bar)
> +! { dg-error "'detach' clause argument not pointer or allocatable" "" {
> target *-*-* } .-1 }
> +!$acc exit data detach(var%foo)
> +! { dg-error "'detach' clause argument not pointer or allocatable" "" {
> target *-*-* } .-1 }
> +
> +!$acc exit data copyout(var)
> +
> +if (var%foo.ne.5) stop 1
> +if (var%bar.ne."GOODBYE!") stop 2
> +
> +end
-
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: Further attach/detach clause fixes for Fortran [PR109622]
On 29.04.23 12:57, Julian Brown wrote:
This patch moves several tests introduced by the following patch:
https://gcc.gnu.org/pipermail/gcc-patches/2023-April/616939.html
I believe you intent this as git log entry. Can you add
... commit r14-325-gcacf65d74463600815773255e8b82b4043432bd7
as this makes looking at the git history easier.
into the proper location for OpenACC testing (thanks to Thomas for
spotting my mistake!), and also fixes a few additional problems --
missing diagnostics for non-pointer attaches, and a case where a pointer
was incorrectly dereferenced. Tests are also adjusted for vector-length
warnings on nvidia accelerators.
Tested with offloading to nvptx. OK?
2023-04-29 Julian Brown
PR fortran/109622
gcc/fortran/
* trans-openmp.cc (gfc_trans_omp_clauses): Add diagnostic for
non-pointer/non-allocatable attach/detach. Remove dereference for
pointer-to-scalar derived type component attach/detach.
In general, we prefer resolution-time diagnostic to tree-translation diagnostic,
unless there is a good reason to do the latter.
At a glance, it should be even sufficient to have a single diagnostic
instead of two when placed into openmp.cc.
Search for lastref in resolve_omp_clauses; I think it should do,
but otherwise something like:
symbol_attr attr = gfc_expr_attr(e);
if (attr.pointer || attr.allocatable)
should work.
You currently have:
@@ -3430,6 +3432,13 @@ gfc_trans_omp_clauses (stmtblock_t *block,
gfc_omp_clauses *clauses,
= TYPE_SIZE_UNIT (gfc_charlen_type_node);
}
}
+ else if (openacc
+&& (n->u.map_op == OMP_MAP_ATTACH
+|| n->u.map_op == OMP_MAP_DETACH))
+ gfc_error ("%qs clause argument not pointer or "
+"allocatable at %L",
+(n->u.map_op == OMP_MAP_ATTACH)
+? "attach" : "detach", );
Additionally, I think we we usually have wording like: 'must be ALLOCATABLE or
a POINTER'.
(Which avoids also the question whether 'neither' instead of 'not should be used
and/besides to 'nor' instead of 'or'.)
Additionally, I think there should be a also an error for:
integer :: a
!$acc enter data attach(a)
end
(Well, in that case, just looking at n->expr won't work, but also n->sym needs
to be
handled for list == OMP_LIST_MAP && n->u.map_op == OMP_MAP_(DE)ATTACH.
The other changes look fine. Thanks!
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
[PATCH] OpenACC: Further attach/detach clause fixes for Fortran [PR109622]
This patch moves several tests introduced by the following patch:
https://gcc.gnu.org/pipermail/gcc-patches/2023-April/616939.html
into the proper location for OpenACC testing (thanks to Thomas for
spotting my mistake!), and also fixes a few additional problems --
missing diagnostics for non-pointer attaches, and a case where a pointer
was incorrectly dereferenced. Tests are also adjusted for vector-length
warnings on nvidia accelerators.
Tested with offloading to nvptx. OK?
2023-04-29 Julian Brown
PR fortran/109622
gcc/fortran/
* trans-openmp.cc (gfc_trans_omp_clauses): Add diagnostic for
non-pointer/non-allocatable attach/detach. Remove dereference for
pointer-to-scalar derived type component attach/detach.
gcc/testsuite/
* gfortran.dg/goacc/pr109622-5.f90: New test.
libgomp/
* testsuite/libgomp.fortran/pr109622.f90: Move test...
* testsuite/libgomp.oacc-fortran/pr109622.f90: ...to here. Ignore
vector length warning.
* testsuite/libgomp.fortran/pr109622-2.f90: Move test...
* testsuite/libgomp.oacc-fortran/pr109622-2.f90: ...to here. Add
missing copyin/copyout variable. Ignore vector length warnings.
* testsuite/libgomp.fortran/pr109622-3.f90: Move test...
* testsuite/libgomp.oacc-fortran/pr109622-3.f90: ...to here. Ignore
vector length warnings.
* testsuite/libgomp.oacc-fortran/pr109622-4.f90: New test.
---
gcc/fortran/trans-openmp.cc | 38 ---
.../gfortran.dg/goacc/pr109622-5.f90 | 45 ++
.../pr109622-2.f90| 7 ++-
.../pr109622-3.f90| 3 ++
.../libgomp.oacc-fortran/pr109622-4.f90 | 47 +++
.../pr109622.f90 | 3 ++
6 files changed, 135 insertions(+), 8 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/goacc/pr109622-5.f90
rename libgomp/testsuite/{libgomp.fortran =>
libgomp.oacc-fortran}/pr109622-2.f90 (63%)
rename libgomp/testsuite/{libgomp.fortran =>
libgomp.oacc-fortran}/pr109622-3.f90 (76%)
create mode 100644 libgomp/testsuite/libgomp.oacc-fortran/pr109622-4.f90
rename libgomp/testsuite/{libgomp.fortran =>
libgomp.oacc-fortran}/pr109622.f90 (78%)
diff --git a/gcc/fortran/trans-openmp.cc b/gcc/fortran/trans-openmp.cc
index 6ee22faa836a..b9a4ae3e53a8 100644
--- a/gcc/fortran/trans-openmp.cc
+++ b/gcc/fortran/trans-openmp.cc
@@ -3395,6 +3395,8 @@ gfc_trans_omp_clauses (stmtblock_t *block,
gfc_omp_clauses *clauses,
&& (n->u.map_op == OMP_MAP_ATTACH
|| n->u.map_op == OMP_MAP_DETACH))
{
+ OMP_CLAUSE_DECL (node)
+ = build_fold_addr_expr (OMP_CLAUSE_DECL (node));
OMP_CLAUSE_SIZE (node) = size_zero_node;
goto finalize_map_clause;
}
@@ -3430,6 +3432,13 @@ gfc_trans_omp_clauses (stmtblock_t *block,
gfc_omp_clauses *clauses,
= TYPE_SIZE_UNIT (gfc_charlen_type_node);
}
}
+ else if (openacc
+ && (n->u.map_op == OMP_MAP_ATTACH
+ || n->u.map_op == OMP_MAP_DETACH))
+ gfc_error ("%qs clause argument not pointer or "
+ "allocatable at %L",
+ (n->u.map_op == OMP_MAP_ATTACH)
+ ? "attach" : "detach", );
}
else if (n->expr
&& n->expr->expr_type == EXPR_VARIABLE
@@ -3510,6 +3519,13 @@ gfc_trans_omp_clauses (stmtblock_t *block,
gfc_omp_clauses *clauses,
}
else
{
+ if (openacc
+ && (n->u.map_op == OMP_MAP_ATTACH
+ || n->u.map_op == OMP_MAP_DETACH))
+ gfc_error ("%qs clause argument not pointer or "
+ "allocatable at %L",
+ (n->u.map_op == OMP_MAP_ATTACH)
+ ? "attach" : "detach", );
OMP_CLAUSE_DECL (node) = inner;
OMP_CLAUSE_SIZE (node)
= TYPE_SIZE_UNIT (TREE_TYPE (inner));
@@ -3523,15 +3539,25 @@ 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)))
{
-
