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 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,
&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;
+ 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", &n->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;
}
@@ -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))
+
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,
&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
finalization issue
All, PR97122 shows an issue with finalization. It seems I attached a diff to the PR in 2020, which allows the code to compile. The diff applied to today's trunk still allows the code to compile but there is now at least 1 regression with finalize_8.f03. Even with my patch, I'm uncertain as to whether the finalizing subroutine is correctly hooked to the right namespace. -- steve
