diff mbox series

[committed,PR90921] Fortran OpenACC 'declare' directive's module handling causes duplicate data clauses (was: [PATCH, OpenACC] Fortran "declare create"/allocate support for OpenACC)

Message ID 87h88mfq8k.fsf@euler.schwinge.homeip.net
State New
Headers show
Series [committed,PR90921] Fortran OpenACC 'declare' directive's module handling causes duplicate data clauses (was: [PATCH, OpenACC] Fortran "declare create"/allocate support for OpenACC) | expand

Commit Message

Thomas Schwinge June 18, 2019, 11:06 p.m. UTC 
Hi!

On Thu, 4 Oct 2018 14:04:13 +0100, Julian Brown <julian@codesourcery.com> wrote:
> On Sun, 23 Sep 2018 10:48:52 +0200
> Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:
> 
> > On Sat, 22 Sep 2018 at 00:32, Julian Brown <julian@codesourcery.com>
> > wrote:
> > 
> > @@ -6218,13 +6221,20 @@ add_clause (gfc_symbol *sym, gfc_omp_map_op
> > map_op) {
> >    gfc_omp_namelist *n;
> > 
> > +  if (!module_oacc_clauses)
> > +    module_oacc_clauses = gfc_get_omp_clauses ();
> > +
> > +  if (sym->backend_decl == NULL)
> > +    gfc_get_symbol_decl (sym);
> > +
> > +  for (n = module_oacc_clauses->lists[OMP_LIST_MAP]; n != NULL; n =
> > n->next)
> > +    if (n->sym->backend_decl == sym->backend_decl)
> > +      return;
> > +
> > 
> > Didn't look too close, but should this throw an error instead of
> > silently returning, or was the error emitted earlier?

Bernhard, thanks for pointing out this "smelly" code, and then Julian for
analyzing the actual issue:

> The purpose of this fragment seems not to have been to do with error
> reporting at all, but rather to do with de-duplicating symbols that
> are listed (once) in clauses of "declare" directives in module blocks.
> Variables that are listed twice are diagnosed elsewhere.
> 
> As for why the de-duplication is necessary, it seems to be because of
> the way that modules are instantiated in programs and in subroutines.
> E.g. in declare-allocatable-1.f90, we have something along the lines of:
> 
>   module vars
>     implicit none
>     integer, parameter :: n = 100
>     real*8, allocatable :: b(:)
>    !$acc declare create (b)
>   end module vars
> 
>   program test
>     use vars
>     ...
>   end program test
> 
>   subroutine sub1
>     use vars
>     ...
>   end subroutine sub1
> 
>   subroutine sub2
>     use vars
>     ...
>   end subroutine sub2
> 
> The function find_module_oacc_declare_clauses is called for each of
> 'test', 'sub1' and 'sub2'. But in trans-decl.c:finish_oacc_declare, the
> new declare clauses are only attached to the namespace for a FL_PROGRAM
> (i.e. 'test'), not for the subroutines. The module_oacc_clauses global
> variable is reset only after moving the clauses to a FL_PROGRAM's
> namespace, otherwise it accumulates.
> 
> Hence, with the above code, we'd scan 'test', find declare clauses, and
> attach them to the namespace for 'test'. We'd then reset
> module_oacc_clauses.
> 
> Then, we'd scan 'sub1', and accumulate declare clauses from 'vars' into
> a fresh module_oacc_clauses.
> 
> Then we'd scan 'sub2', and accumulate declare clauses from 'vars'
> again: this is why the de-duplication in the patch seemed to be
> necessary.
> 
> This seems wrong to me though, and admits the possibility of clauses
> instantiated in a subroutine "leaking" into a subsequent program block.
> As a tentative fix, I've tried resetting module_oacc_clauses before
> each time the find_module_oacc_declare_clauses traversal takes place,
> and removing the de-duplication code.

So, that's clearly a separate bug from everything else discussed as part
of this patch submission; <https://gcc.gnu.org/PR90921> "Fortran OpenACC
'declare' directive's module handling causes duplicate data clauses"
filed.

> This seems to work fine for the current tests in the testsuite, but I
> wonder the reason that things weren't done like like that to start
> with? The code dates back to 2015 (by James Norris):
> 
> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg02367.html

There remains a lot of mystery to be resolved regarding the OpenACC
'declare' implementation...  :-(

> --- a/gcc/fortran/trans-decl.c
> +++ b/gcc/fortran/trans-decl.c
> @@ -6272,6 +6275,8 @@ finish_oacc_declare (gfc_namespace *ns, gfc_symbol *sym, bool block)
>    gfc_omp_clauses *omp_clauses = NULL;
>    gfc_omp_namelist *n, *p;
>  
> +  module_oacc_clauses = NULL;
> +
>    gfc_traverse_ns (ns, find_module_oacc_declare_clauses);
>  
>    if (module_oacc_clauses && sym->attr.flavor == FL_PROGRAM)
> @@ -6283,7 +6288,6 @@ finish_oacc_declare (gfc_namespace *ns, gfc_symbol *sym, bool block)
>        new_oc->clauses = module_oacc_clauses;
>  
>        ns->oacc_declare = new_oc;
> -      module_oacc_clauses = NULL;
>      }
>  
>    if (!ns->oacc_declare)

I cannot claim to understand this Fortran OpenACC 'declare' directive's
module handling here, but I can at least confirm via a test case that
I've added, that your change makes the duplicate data clauses go away;
committed to trunk in r272454 "[PR90921] Fortran OpenACC 'declare'
directive's module handling causes duplicate data clauses", see attached.


Grüße
 Thomas
diff mbox series

Patch

From 9f15ed31065cf6baaae9b3e0e4c16fb9e958fbd9 Mon Sep 17 00:00:00 2001
From: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4>
Date: Tue, 18 Jun 2019 22:15:53 +0000
Subject: [PATCH] [PR90921] Fortran OpenACC 'declare' directive's module
 handling causes duplicate data clauses

	gcc/fortran/
	PR fortran/90921
	* trans-decl.c (finish_oacc_declare): Reset module_oacc_clauses
	before scanning each namespace.
	gcc/testsuite/
	PR fortran/90921
	* gfortran.dg/goacc/declare-3.f95: Update.

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@272454 138bc75d-0d04-0410-961f-82ee72b054a4
---
 gcc/fortran/ChangeLog                         | 6 ++++++
 gcc/fortran/trans-decl.c                      | 2 +-
 gcc/testsuite/ChangeLog                       | 3 +++
 gcc/testsuite/gfortran.dg/goacc/declare-3.f95 | 6 ++++++
 4 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/gcc/fortran/ChangeLog b/gcc/fortran/ChangeLog
index 6fd97b61ce05..32d961ade960 100644
--- a/gcc/fortran/ChangeLog
+++ b/gcc/fortran/ChangeLog
@@ -1,3 +1,9 @@ 
+2019-06-18  Julian Brown  <julian@codesourcery.com>
+
+	PR fortran/90921
+	* trans-decl.c (finish_oacc_declare): Reset module_oacc_clauses
+	before scanning each namespace.
+
 2019-06-18  Thomas Schwinge  <thomas@codesourcery.com>
 
 	PR fortran/85221
diff --git a/gcc/fortran/trans-decl.c b/gcc/fortran/trans-decl.c
index f504c279c31b..64ce4bba23d9 100644
--- a/gcc/fortran/trans-decl.c
+++ b/gcc/fortran/trans-decl.c
@@ -6491,6 +6491,7 @@  finish_oacc_declare (gfc_namespace *ns, gfc_symbol *sym, bool block)
   gfc_omp_clauses *omp_clauses = NULL;
   gfc_omp_namelist *n, *p;
 
+  module_oacc_clauses = NULL;
   gfc_traverse_ns (ns, find_module_oacc_declare_clauses);
 
   if (module_oacc_clauses && sym->attr.flavor == FL_PROGRAM)
@@ -6502,7 +6503,6 @@  finish_oacc_declare (gfc_namespace *ns, gfc_symbol *sym, bool block)
       new_oc->clauses = module_oacc_clauses;
 
       ns->oacc_declare = new_oc;
-      module_oacc_clauses = NULL;
     }
 
   if (!ns->oacc_declare)
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 552ccc6fbd68..6ff197c8e4dc 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,5 +1,8 @@ 
 2019-06-18  Thomas Schwinge  <thomas@codesourcery.com>
 
+	PR fortran/90921
+	* gfortran.dg/goacc/declare-3.f95: Update.
+
 	PR fortran/85221
 	* gfortran.dg/goacc/declare-3.f95: New file.
 
diff --git a/gcc/testsuite/gfortran.dg/goacc/declare-3.f95 b/gcc/testsuite/gfortran.dg/goacc/declare-3.f95
index ec5d4c5a062a..80d9903a9dc6 100644
--- a/gcc/testsuite/gfortran.dg/goacc/declare-3.f95
+++ b/gcc/testsuite/gfortran.dg/goacc/declare-3.f95
@@ -1,5 +1,7 @@ 
 ! Test valid usage of the OpenACC 'declare' directive.
 
+! { dg-additional-options "-fdump-tree-original" }
+
 module mod_a
   implicit none
   integer :: a
@@ -44,4 +46,8 @@  program test
   use mod_c
   use mod_d
   use mod_e
+
+  ! { dg-final { scan-tree-dump {(?n)#pragma acc data map\(force_alloc:d\) map\(force_deviceptr:c\) map\(force_to:b\) map\(force_alloc:a\)$} original } }
 end program test
+
+! { dg-final { scan-tree-dump-times {#pragma acc data} 1 original } }
-- 
2.20.1