[PATCH][OpenMP] Fix declare target variables in fortran modules

[PATCH][OpenMP] Fix declare target variables in fortran modules

[Ilya Verbin]

Hi,

We have a problem with declare target variables in fortran modules, here is a
small reproducer:

+++++ share.f90:

module share
  integer :: var_x
!$omp declare target(var_x)
end module

+++++ test.f90:

  use share
  var_x = 10
!$omp target update to(var_x)
end

+++++

$ gfortran -fopenmp -c share.f90
$ gfortran -fopenmp -c test.f90
$ gfortran -fopenmp share.o test.o
$ ./a.out
libgomp: Duplicate node

+++++

This happens because the var_x is added into offload tables for both share.o and
test.o.  The patch below fixes this issue.  Regtested on x86_64-linux and
i686-linux.  However I'm not sure how to create a regression test, which would
compile 2 separate objects, and check run-time result.


diff --git a/gcc/varpool.c b/gcc/varpool.c
index 707f62f..5929d92 100644
--- a/gcc/varpool.c
+++ b/gcc/varpool.c
@@ -173,7 +173,7 @@ varpool_node::get_create (tree decl)
   node = varpool_node::create_empty ();
   node->decl = decl;
 
-  if ((flag_openacc || flag_openmp)
+  if ((flag_openacc || flag_openmp) && !DECL_EXTERNAL (decl)
       && lookup_attribute ("omp declare target", DECL_ATTRIBUTES (decl)))
     {
       node->offloadable = 1;


Thanks,
  -- Ilya

Re: [PATCH][OpenMP] Fix declare target variables in fortran modules

[Jakub Jelinek]

On Thu, Mar 12, 2015 at 04:56:35PM +0300, Ilya Verbin wrote:
> This happens because the var_x is added into offload tables for both share.o and
> test.o.  The patch below fixes this issue.  Regtested on x86_64-linux and
> i686-linux.  However I'm not sure how to create a regression test, which would
> compile 2 separate objects, and check run-time result.

Ok with proper ChangeLog entry.

As for testcase, won't dg-additional-sources help?
I mean, does it fail without your patch even if you just do
gfortran -fopenmp -o a.out share.f90 test.f90; ./a.out ?

> --- a/gcc/varpool.c
> +++ b/gcc/varpool.c
> @@ -173,7 +173,7 @@ varpool_node::get_create (tree decl)
>    node = varpool_node::create_empty ();
>    node->decl = decl;
>  
> -  if ((flag_openacc || flag_openmp)
> +  if ((flag_openacc || flag_openmp) && !DECL_EXTERNAL (decl)
>        && lookup_attribute ("omp declare target", DECL_ATTRIBUTES (decl)))
>      {
>        node->offloadable = 1;

	Jakub

Re: [PATCH][OpenMP] Fix declare target variables in fortran modules

[Ilya Verbin]

On Thu, Mar 12, 2015 at 15:21:35 +0100, Jakub Jelinek wrote:
> On Thu, Mar 12, 2015 at 04:56:35PM +0300, Ilya Verbin wrote:
> > This happens because the var_x is added into offload tables for both share.o and
> > test.o.  The patch below fixes this issue.  Regtested on x86_64-linux and
> > i686-linux.  However I'm not sure how to create a regression test, which would
> > compile 2 separate objects, and check run-time result.
> 
> Ok with proper ChangeLog entry.
> 
> As for testcase, won't dg-additional-sources help?
> I mean, does it fail without your patch even if you just do
> gfortran -fopenmp -o a.out share.f90 test.f90; ./a.out ?

Yes, this works.  Here is what I will commit tomorrow, if no objections.


gcc/
	* varpool.c (varpool_node::get_create): Don't set 'offloadable' flag for
	the external decls.
libgomp/
	* testsuite/libgomp.fortran/declare-target-1.f90: New test.
	* testsuite/libgomp.fortran/declare-target-2.f90: New file.


diff --git a/gcc/varpool.c b/gcc/varpool.c
index b583693..ce64279 100644
--- a/gcc/varpool.c
+++ b/gcc/varpool.c
@@ -173,7 +173,7 @@ varpool_node::get_create (tree decl)
   node = varpool_node::create_empty ();
   node->decl = decl;
 
-  if ((flag_openacc || flag_openmp)
+  if ((flag_openacc || flag_openmp) && !DECL_EXTERNAL (decl)
       && lookup_attribute ("omp declare target", DECL_ATTRIBUTES (decl)))
     {
       node->offloadable = 1;
diff --git a/libgomp/testsuite/libgomp.fortran/declare-target-1.f90 b/libgomp/testsuite/libgomp.fortran/declare-target-1.f90
new file mode 100644
index 0000000..fd9c26f
--- /dev/null
+++ b/libgomp/testsuite/libgomp.fortran/declare-target-1.f90
@@ -0,0 +1,15 @@
+! { dg-do run }
+! { dg-additional-sources declare-target-2.f90 }
+
+module declare_target_1_mod
+  integer :: var_x
+  !$omp declare target(var_x)
+end module declare_target_1_mod
+
+  interface
+    subroutine foo ()
+    end subroutine foo
+  end interface
+
+  call foo ()
+end
diff --git a/libgomp/testsuite/libgomp.fortran/declare-target-2.f90 b/libgomp/testsuite/libgomp.fortran/declare-target-2.f90
new file mode 100644
index 0000000..f8d3ab2
--- /dev/null
+++ b/libgomp/testsuite/libgomp.fortran/declare-target-2.f90
@@ -0,0 +1,18 @@
+! Don't compile this anywhere, it is just auxiliary
+! file compiled together with declare-target-1.f90
+! to verify inter-CU module handling of omp declare target.
+! { dg-do compile { target { lp64 && { ! lp64 } } } }
+
+subroutine foo
+  use declare_target_1_mod
+
+  var_x = 10
+  !$omp target update to(var_x)
+
+  !$omp target
+    var_x = var_x * 2;
+  !$omp end target
+
+  !$omp target update from(var_x)
+  if (var_x /= 20) call abort
+end subroutine foo


  -- Ilya

Re: [PATCH][OpenMP] Fix declare target variables in fortran modules

[Jakub Jelinek]

On Thu, Mar 12, 2015 at 10:22:37PM +0300, Ilya Verbin wrote:
> On Thu, Mar 12, 2015 at 15:21:35 +0100, Jakub Jelinek wrote:
> > On Thu, Mar 12, 2015 at 04:56:35PM +0300, Ilya Verbin wrote:
> > > This happens because the var_x is added into offload tables for both share.o and
> > > test.o.  The patch below fixes this issue.  Regtested on x86_64-linux and
> > > i686-linux.  However I'm not sure how to create a regression test, which would
> > > compile 2 separate objects, and check run-time result.
> > 
> > Ok with proper ChangeLog entry.
> > 
> > As for testcase, won't dg-additional-sources help?
> > I mean, does it fail without your patch even if you just do
> > gfortran -fopenmp -o a.out share.f90 test.f90; ./a.out ?
> 
> Yes, this works.  Here is what I will commit tomorrow, if no objections.

Ok, thanks.

> gcc/
> 	* varpool.c (varpool_node::get_create): Don't set 'offloadable' flag for
> 	the external decls.
> libgomp/
> 	* testsuite/libgomp.fortran/declare-target-1.f90: New test.
> 	* testsuite/libgomp.fortran/declare-target-2.f90: New file.
> 
> 
> diff --git a/gcc/varpool.c b/gcc/varpool.c
> index b583693..ce64279 100644
> --- a/gcc/varpool.c
> +++ b/gcc/varpool.c
> @@ -173,7 +173,7 @@ varpool_node::get_create (tree decl)
>    node = varpool_node::create_empty ();
>    node->decl = decl;
>  
> -  if ((flag_openacc || flag_openmp)
> +  if ((flag_openacc || flag_openmp) && !DECL_EXTERNAL (decl)
>        && lookup_attribute ("omp declare target", DECL_ATTRIBUTES (decl)))
>      {
>        node->offloadable = 1;
> diff --git a/libgomp/testsuite/libgomp.fortran/declare-target-1.f90 b/libgomp/testsuite/libgomp.fortran/declare-target-1.f90
> new file mode 100644
> index 0000000..fd9c26f
> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.fortran/declare-target-1.f90
> @@ -0,0 +1,15 @@
> +! { dg-do run }
> +! { dg-additional-sources declare-target-2.f90 }
> +
> +module declare_target_1_mod
> +  integer :: var_x
> +  !$omp declare target(var_x)
> +end module declare_target_1_mod
> +
> +  interface
> +    subroutine foo ()
> +    end subroutine foo
> +  end interface
> +
> +  call foo ()
> +end
> diff --git a/libgomp/testsuite/libgomp.fortran/declare-target-2.f90 b/libgomp/testsuite/libgomp.fortran/declare-target-2.f90
> new file mode 100644
> index 0000000..f8d3ab2
> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.fortran/declare-target-2.f90
> @@ -0,0 +1,18 @@
> +! Don't compile this anywhere, it is just auxiliary
> +! file compiled together with declare-target-1.f90
> +! to verify inter-CU module handling of omp declare target.
> +! { dg-do compile { target { lp64 && { ! lp64 } } } }
> +
> +subroutine foo
> +  use declare_target_1_mod
> +
> +  var_x = 10
> +  !$omp target update to(var_x)
> +
> +  !$omp target
> +    var_x = var_x * 2;
> +  !$omp end target
> +
> +  !$omp target update from(var_x)
> +  if (var_x /= 20) call abort
> +end subroutine foo
> 
> 
>   -- Ilya

	Jakub