* [Patch] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR11555]
@ 2024-06-21 13:06 Tobias Burnus
2024-07-26 18:34 ` [Patch, v2] " Tobias Burnus
0 siblings, 1 reply; 8+ messages in thread
From: Tobias Burnus @ 2024-06-21 13:06 UTC (permalink / raw)
To: gcc-patches, Jakub Jelinek, fortran
[-- Attachment #1.1: Type: text/plain, Size: 1399 bytes --]
Hi all,
it turned out that 'declare target' with 'link' clause was broken in multiple ways.
The main fix is the attached patch, i.e. namely pushing the variables already to
the offload-vars list already in the FE.
When implementing it, I noticed:
* C has a similar issue when using nested functions, which is
a GNU extension →https://gcc.gnu.org/115574
* When doing partial mapping of arrays (which is one of the reasons for 'link'),
offsets are mishandled in Fortran (not tested in C), see FIXME in the patch)
There: arr2(10) should print 10 but with map(arr2(10:)) it prints 19.
(I will file a PR about this).
* It might happen that linked variables do not get linked. I have not investigated
why, but 'arr2' gives link errors – while 'arr' works.
See FIXME in the patch. (I will file a PR about this)
* For COMMON blocks, map(/common/) is rejected,https://gcc.gnu.org/PR115577
* When then mapping map(a,b,c) which is identical for 'common /mycom/ a,b,c',
it fails to link the device side as the 'mycom_' symbol cannot be found on the
device side. (I will file a PR about this)
As COMMON as issues, an alternative would be to defer the trans-common.cc
changes to a later patch.
Comments, questions, concerns?
Tobias
PS: Tested with nvptx offloading with a page-migration supporting system with
nvptx and GCN offloading configured and no new fails observed.
[-- Attachment #2: omp-declare-tgt-link-v2.diff --]
[-- Type: text/x-patch, Size: 12376 bytes --]
OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR11555]
Contrary to a normal 'declare target', the 'declare target link' attribute
also needs to set node->offloadable and push the offload_vars in the front end.
Linked variables require that the data is mapped. For module variables, this
can happen anywhere. For variables in an external subprograms or the main
programm, this can only happen in the either that program itself or in an
internal subprogram. - Whether a variable is just normally mapped or linked then
becomes relevant if a device routine exists that can access that variable,
i.e. an internal procedure has then to be marked as declare target.
PR fortran/115559
gcc/fortran/ChangeLog:
* trans-common.cc (build_common_decl): Add 'omp declare target' and
'omp declare target link' variables to offload_vars.
* trans-decl.cc (add_attributes_to_decl): Likewise; update args and
call decl_attributes.
(get_proc_pointer_decl, gfc_get_extern_function_decl,
build_function_decl): Update calls.
(gfc_get_symbol_decl): Likewise; move after 'DECL_STATIC (t)=1'
to avoid errors with symtab_node::get_create.
libgomp/ChangeLog:
* testsuite/libgomp.fortran/declare-target-link.f90: New test.
gcc/fortran/trans-common.cc | 21 ++++
gcc/fortran/trans-decl.cc | 81 +++++++++-----
.../libgomp.fortran/declare-target-link.f90 | 119 +++++++++++++++++++++
3 files changed, 195 insertions(+), 26 deletions(-)
diff --git a/gcc/fortran/trans-common.cc b/gcc/fortran/trans-common.cc
index 5f44e7bd663..e714342c3c0 100644
--- a/gcc/fortran/trans-common.cc
+++ b/gcc/fortran/trans-common.cc
@@ -98,6 +98,9 @@ along with GCC; see the file COPYING3. If not see
#include "coretypes.h"
#include "tm.h"
#include "tree.h"
+#include "cgraph.h"
+#include "context.h"
+#include "omp-offload.h"
#include "gfortran.h"
#include "trans.h"
#include "stringpool.h"
@@ -497,6 +500,24 @@ build_common_decl (gfc_common_head *com, tree union_type, bool is_init)
= tree_cons (get_identifier ("omp declare target"),
omp_clauses, DECL_ATTRIBUTES (decl));
+ if (com->omp_declare_target_link || com->omp_declare_target)
+ {
+ /* Add to offload_vars; get_create does so for omp_declare_target,
+ omp_declare_target_link requires manual work. */
+ gcc_assert (symtab_node::get (decl) == 0);
+ symtab_node *node = symtab_node::get_create (decl);
+ if (node != NULL && com->omp_declare_target_link)
+ {
+ node->offloadable = 1;
+ if (ENABLE_OFFLOADING)
+ {
+ g->have_offload = true;
+ if (is_a <varpool_node *> (node))
+ vec_safe_push (offload_vars, decl);
+ }
+ }
+ }
+
/* Place the back end declaration for this common block in
GLOBAL_BINDING_LEVEL. */
gfc_map_of_all_commons[identifier] = pushdecl_top_level (decl);
diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
index 8d4f06a4e1d..4067dd6ed77 100644
--- a/gcc/fortran/trans-decl.cc
+++ b/gcc/fortran/trans-decl.cc
@@ -46,7 +46,9 @@ along with GCC; see the file COPYING3. If not see
#include "trans-stmt.h"
#include "gomp-constants.h"
#include "gimplify.h"
+#include "context.h"
#include "omp-general.h"
+#include "omp-offload.h"
#include "attr-fnspec.h"
#include "tree-iterator.h"
#include "dependency.h"
@@ -1470,19 +1472,18 @@ gfc_add_assign_aux_vars (gfc_symbol * sym)
}
-static tree
-add_attributes_to_decl (symbol_attribute sym_attr, tree list)
+static void
+add_attributes_to_decl (tree *decl_p, const gfc_symbol *sym)
{
unsigned id;
- tree attr;
+ tree list = NULL_TREE;
+ symbol_attribute sym_attr = sym->attr;
for (id = 0; id < EXT_ATTR_NUM; id++)
if (sym_attr.ext_attr & (1 << id) && ext_attr_list[id].middle_end_name)
{
- attr = build_tree_list (
- get_identifier (ext_attr_list[id].middle_end_name),
- NULL_TREE);
- list = chainon (list, attr);
+ tree ident = get_identifier (ext_attr_list[id].middle_end_name);
+ list = tree_cons (ident, NULL_TREE, list);
}
tree clauses = NULL_TREE;
@@ -1545,6 +1546,7 @@ add_attributes_to_decl (symbol_attribute sym_attr, tree list)
clauses = c;
}
+ bool has_declare = true;
if (sym_attr.omp_declare_target_link
|| sym_attr.oacc_declare_link)
list = tree_cons (get_identifier ("omp declare target link"),
@@ -1556,12 +1558,45 @@ add_attributes_to_decl (symbol_attribute sym_attr, tree list)
|| sym_attr.oacc_declare_device_resident)
list = tree_cons (get_identifier ("omp declare target"),
clauses, list);
+ else
+ has_declare = false;
if (sym_attr.omp_declare_target_indirect)
list = tree_cons (get_identifier ("omp declare target indirect"),
clauses, list);
- return list;
+ decl_attributes (decl_p, list, 0);
+
+ if (has_declare
+ && VAR_P (*decl_p)
+ && sym->ns->proc_name->attr.flavor != FL_MODULE)
+ {
+ has_declare = false;
+ for (gfc_namespace* ns = sym->ns->contained; ns; ns = ns->sibling)
+ if (ns->proc_name->attr.omp_declare_target)
+ {
+ has_declare = true;
+ break;
+ }
+ }
+
+ if (has_declare && VAR_P (*decl_p) && has_declare)
+ {
+ /* Add to offload_vars; get_create does so for omp_declare_target,
+ omp_declare_target_link requires manual work. */
+ gcc_assert (symtab_node::get (*decl_p) == 0);
+ symtab_node *node = symtab_node::get_create (*decl_p);
+ if (node != NULL && sym_attr.omp_declare_target_link)
+ {
+ node->offloadable = 1;
+ if (ENABLE_OFFLOADING)
+ {
+ g->have_offload = true;
+ if (is_a <varpool_node *> (node))
+ vec_safe_push (offload_vars, *decl_p);
+ }
+ }
+ }
}
@@ -1576,7 +1611,6 @@ gfc_get_symbol_decl (gfc_symbol * sym)
{
tree decl;
tree length = NULL_TREE;
- tree attributes;
int byref;
bool intrinsic_array_parameter = false;
bool fun_or_res;
@@ -1861,12 +1895,6 @@ gfc_get_symbol_decl (gfc_symbol * sym)
decl = build_decl (gfc_get_location (&sym->declared_at),
VAR_DECL, gfc_sym_identifier (sym), gfc_sym_type (sym));
- /* Add attributes to variables. Functions are handled elsewhere. */
- attributes = add_attributes_to_decl (sym->attr, NULL_TREE);
- decl_attributes (&decl, attributes, 0);
- if (sym->ts.deferred && VAR_P (length))
- decl_attributes (&length, attributes, 0);
-
/* Symbols from modules should have their assembler names mangled.
This is done here rather than in gfc_finish_var_decl because it
is different for string length variables. */
@@ -2032,6 +2060,12 @@ gfc_get_symbol_decl (gfc_symbol * sym)
TREE_READONLY (decl) = 1;
}
+ /* Add attributes to variables. Functions are handled elsewhere. */
+ add_attributes_to_decl (&decl, sym);
+
+ if (sym->ts.deferred && VAR_P (length))
+ decl_attributes (&length, DECL_ATTRIBUTES (decl), 0);
+
return decl;
}
@@ -2068,7 +2102,6 @@ static tree
get_proc_pointer_decl (gfc_symbol *sym)
{
tree decl;
- tree attributes;
if (sym->module || sym->fn_result_spec)
{
@@ -2148,8 +2181,7 @@ get_proc_pointer_decl (gfc_symbol *sym)
&& (TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
set_decl_tls_model (decl, decl_default_tls_model (decl));
- attributes = add_attributes_to_decl (sym->attr, NULL_TREE);
- decl_attributes (&decl, attributes, 0);
+ add_attributes_to_decl (&decl, sym);
return decl;
}
@@ -2163,7 +2195,6 @@ gfc_get_extern_function_decl (gfc_symbol * sym, gfc_actual_arglist *actual_args,
{
tree type;
tree fndecl;
- tree attributes;
gfc_expr e;
gfc_intrinsic_sym *isym;
gfc_expr argexpr;
@@ -2361,8 +2392,7 @@ module_sym:
DECL_EXTERNAL (fndecl) = 1;
TREE_PUBLIC (fndecl) = 1;
- attributes = add_attributes_to_decl (sym->attr, NULL_TREE);
- decl_attributes (&fndecl, attributes, 0);
+ add_attributes_to_decl (&fndecl, sym);
gfc_set_decl_assembler_name (fndecl, mangled_name);
@@ -2421,7 +2451,7 @@ module_sym:
static void
build_function_decl (gfc_symbol * sym, bool global)
{
- tree fndecl, type, attributes;
+ tree fndecl, type;
symbol_attribute attr;
tree result_decl;
gfc_formal_arglist *f;
@@ -2472,15 +2502,14 @@ build_function_decl (gfc_symbol * sym, bool global)
if (sym->attr.referenced || sym->attr.entry_master)
TREE_USED (fndecl) = 1;
- attributes = add_attributes_to_decl (attr, NULL_TREE);
- decl_attributes (&fndecl, attributes, 0);
+ add_attributes_to_decl (&fndecl, sym);
/* Figure out the return type of the declared function, and build a
RESULT_DECL for it. If this is a subroutine with alternate
returns, build a RESULT_DECL for it. */
result_decl = NULL_TREE;
/* TODO: Shouldn't this just be TREE_TYPE (TREE_TYPE (fndecl)). */
- if (attr.function)
+ if (sym->attr.function)
{
if (gfc_return_by_reference (sym))
type = void_type_node;
@@ -2527,7 +2556,7 @@ build_function_decl (gfc_symbol * sym, bool global)
/* Set attributes for PURE functions. A call to a PURE function in the
Fortran 95 sense is both pure and without side effects in the C
sense. */
- if (attr.pure || attr.implicit_pure)
+ if (sym->attr.pure || sym->attr.implicit_pure)
{
/* TODO: check if a pure SUBROUTINE has no INTENT(OUT) arguments
including an alternate return. In that case it can also be
diff --git a/libgomp/testsuite/libgomp.fortran/declare-target-link.f90 b/libgomp/testsuite/libgomp.fortran/declare-target-link.f90
new file mode 100644
index 00000000000..5243fe09002
--- /dev/null
+++ b/libgomp/testsuite/libgomp.fortran/declare-target-link.f90
@@ -0,0 +1,119 @@
+! { dg-additional-options "-Wall" }
+! PR fortran/115559
+
+module m
+ integer :: A
+ !$omp declare target link(A)
+end module m
+
+subroutine f
+ implicit none (type, external)
+ integer, save :: x, y ! { dg-warning "Unused variable 'y' declared" }
+ !$omp declare target link(x, y)
+
+ ! note: y is not 'link' as gfortran doesn't regard it as used
+ x = 6
+ call ii
+
+contains
+ subroutine k
+ !$omp declare target
+ use m
+ A = 5
+ end
+ subroutine ii
+ integer :: res
+ !$omp target map(x) map(from: res)
+ call k()
+ call ll()
+ res = get()
+ !$omp end target
+ ! print *, res
+ if (res /= 6 + 7 + 5) &
+ stop 1
+ end
+ subroutine ll
+ !$omp declare target
+ x = x + 7
+ end
+ integer function get()
+ use m
+ !$omp declare target
+ get = x + A
+ end
+end
+
+
+subroutine sub
+ implicit none (type, external)
+ integer, save :: arr(100), arr2(1:4)
+ !$omp declare target link(arr,arr2)
+
+ call mapit
+ call device1
+ call re_mapit
+ call device2
+contains
+ subroutine mapit
+ integer :: i
+ arr = [(i, i=1,100)]
+ ! FIXME: map(arr(10:50)) mapping causes offset issues:
+ ! i.e. accessing arr(10) later accesses the 19th not the 10th element!
+ ! !$omp target enter data map(to:arr(10:50)) map(alloc: arr2(1:4))
+ !$omp target enter data map(to:arr) map(alloc: arr2(1:4)) ! Workaround: mapp all of 'arr'
+ end subroutine
+ subroutine re_mapit
+ integer :: i
+ !$omp target exit data map(from:arr(10:50)) map(delete: arr2)
+
+ if (any (arr(1:9) /= [(i, i=1,9)])) stop 2
+ if (any (arr(10:50) /= [(3-10*i, i=10,50)])) stop 3
+ if (any (arr(51:100) /= [(i, i=51,100)])) stop 4
+ end subroutine
+
+ subroutine device1
+ integer :: res
+ !$omp target map(from:res)
+ res = run_device1()
+ !$omp end target
+ print *, res
+ ! FIXME: arr2 not link mapped
+ ! if (res /= -11436) stop 5
+ if (res /= -11546) stop 5 ! FIXME
+ end
+ integer function run_device1()
+ !$omp declare target
+ integer :: i
+ run_device1 = -99
+ ! FIXME: arr2 not link mapped
+ ! arr2 = [11,22,33,44]
+ if (any (arr(10:50) /= [(i, i=10,50)])) then
+ run_device1 = arr(11)
+ return
+ end if
+ ! FIXME:
+ ! run_device1 = sum(arr(10:13) + arr2)
+ run_device1 = sum(arr(10:13) ) ! FIXME
+ do i = 10, 50
+ arr(i) = 3 - 10 * arr(i)
+ end do
+ run_device1 = run_device1 + sum(arr(15:50))
+ end
+ subroutine device2
+ end
+ integer function run_device2()
+ !$omp declare target
+ run_device2 = -99
+ end
+end
+
+
+use m
+implicit none (type, external)
+external f
+external sub
+
+!$omp target enter data map(alloc: A)
+call f()
+call sub
+end
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Patch, v2] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR11555]
2024-06-21 13:06 [Patch] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR11555] Tobias Burnus
@ 2024-07-26 18:34 ` Tobias Burnus
2024-07-29 7:09 ` Andre Vehreschild
2024-07-30 8:37 ` [committed] gfortran.dg/compiler-directive_2.f: Update dg-error (was: [Patch, v2] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR115559]) Tobias Burnus
0 siblings, 2 replies; 8+ messages in thread
From: Tobias Burnus @ 2024-07-26 18:34 UTC (permalink / raw)
To: gcc-patches, Jakub Jelinek, fortran
[-- Attachment #1.1: Type: text/plain, Size: 1761 bytes --]
Updated patch - only change is to the testcase:
* With the just posted patch for PR116107, array sections with offset
work for 'link', hence, I updated the testcase.
* For 'arr2', I added ref to the associated PR.
I intent to commit it once PR116107 has been committed.
Tobias
Tobias Burnus wrote:
> Hi all,
>
> it turned out that 'declare target' with 'link' clause was broken in multiple ways.
>
> The main fix is the attached patch, i.e. namely pushing the variables already to
> the offload-vars list already in the FE.
>
> When implementing it, I noticed:
> * C has a similar issue when using nested functions, which is
> a GNU extension →https://gcc.gnu.org/115574
>
> * When doing partial mapping of arrays (which is one of the reasons for 'link'),
> offsets are mishandled in Fortran (not tested in C), see FIXME in the patch)
> There: arr2(10) should print 10 but with map(arr2(10:)) it prints 19.
> (I will file a PR about this).
>
> * It might happen that linked variables do not get linked. I have not investigated
> why, but 'arr2' gives link errors – while 'arr' works.
> See FIXME in the patch. (I will file a PR about this)
>
> * For COMMON blocks, map(/common/) is rejected,https://gcc.gnu.org/PR115577
>
> * When then mapping map(a,b,c) which is identical for 'common /mycom/ a,b,c',
> it fails to link the device side as the 'mycom_' symbol cannot be found on the
> device side. (I will file a PR about this)
>
> As COMMON as issues, an alternative would be to defer the trans-common.cc
> changes to a later patch.
>
> Comments, questions, concerns?
>
> Tobias
>
> PS: Tested with nvptx offloading with a page-migration supporting system with
> nvptx and GCN offloading configured and no new fails observed.
[-- Attachment #2: omp-declare-tgt-link-v3.diff --]
[-- Type: text/x-patch, Size: 12179 bytes --]
OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR11555]
Contrary to a normal 'declare target', the 'declare target link' attribute
also needs to set node->offloadable and push the offload_vars in the front end.
Linked variables require that the data is mapped. For module variables, this
can happen anywhere. For variables in an external subprograms or the main
programm, this can only happen in the either that program itself or in an
internal subprogram. - Whether a variable is just normally mapped or linked then
becomes relevant if a device routine exists that can access that variable,
i.e. an internal procedure has then to be marked as declare target.
PR fortran/115559
gcc/fortran/ChangeLog:
* trans-common.cc (build_common_decl): Add 'omp declare target' and
'omp declare target link' variables to offload_vars.
* trans-decl.cc (add_attributes_to_decl): Likewise; update args and
call decl_attributes.
(get_proc_pointer_decl, gfc_get_extern_function_decl,
build_function_decl): Update calls.
(gfc_get_symbol_decl): Likewise; move after 'DECL_STATIC (t)=1'
to avoid errors with symtab_node::get_create.
libgomp/ChangeLog:
* testsuite/libgomp.fortran/declare-target-link.f90: New test.
gcc/fortran/trans-common.cc | 21 ++++
gcc/fortran/trans-decl.cc | 81 +++++++++-----
.../libgomp.fortran/declare-target-link.f90 | 116 +++++++++++++++++++++
3 files changed, 192 insertions(+), 26 deletions(-)
diff --git a/gcc/fortran/trans-common.cc b/gcc/fortran/trans-common.cc
index 5f44e7bd663..e714342c3c0 100644
--- a/gcc/fortran/trans-common.cc
+++ b/gcc/fortran/trans-common.cc
@@ -98,6 +98,9 @@ along with GCC; see the file COPYING3. If not see
#include "coretypes.h"
#include "tm.h"
#include "tree.h"
+#include "cgraph.h"
+#include "context.h"
+#include "omp-offload.h"
#include "gfortran.h"
#include "trans.h"
#include "stringpool.h"
@@ -497,6 +500,24 @@ build_common_decl (gfc_common_head *com, tree union_type, bool is_init)
= tree_cons (get_identifier ("omp declare target"),
omp_clauses, DECL_ATTRIBUTES (decl));
+ if (com->omp_declare_target_link || com->omp_declare_target)
+ {
+ /* Add to offload_vars; get_create does so for omp_declare_target,
+ omp_declare_target_link requires manual work. */
+ gcc_assert (symtab_node::get (decl) == 0);
+ symtab_node *node = symtab_node::get_create (decl);
+ if (node != NULL && com->omp_declare_target_link)
+ {
+ node->offloadable = 1;
+ if (ENABLE_OFFLOADING)
+ {
+ g->have_offload = true;
+ if (is_a <varpool_node *> (node))
+ vec_safe_push (offload_vars, decl);
+ }
+ }
+ }
+
/* Place the back end declaration for this common block in
GLOBAL_BINDING_LEVEL. */
gfc_map_of_all_commons[identifier] = pushdecl_top_level (decl);
diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
index 82fa2bb6134..0fdc41b1784 100644
--- a/gcc/fortran/trans-decl.cc
+++ b/gcc/fortran/trans-decl.cc
@@ -46,7 +46,9 @@ along with GCC; see the file COPYING3. If not see
#include "trans-stmt.h"
#include "gomp-constants.h"
#include "gimplify.h"
+#include "context.h"
#include "omp-general.h"
+#include "omp-offload.h"
#include "attr-fnspec.h"
#include "tree-iterator.h"
#include "dependency.h"
@@ -1472,19 +1474,18 @@ gfc_add_assign_aux_vars (gfc_symbol * sym)
}
-static tree
-add_attributes_to_decl (symbol_attribute sym_attr, tree list)
+static void
+add_attributes_to_decl (tree *decl_p, const gfc_symbol *sym)
{
unsigned id;
- tree attr;
+ tree list = NULL_TREE;
+ symbol_attribute sym_attr = sym->attr;
for (id = 0; id < EXT_ATTR_NUM; id++)
if (sym_attr.ext_attr & (1 << id) && ext_attr_list[id].middle_end_name)
{
- attr = build_tree_list (
- get_identifier (ext_attr_list[id].middle_end_name),
- NULL_TREE);
- list = chainon (list, attr);
+ tree ident = get_identifier (ext_attr_list[id].middle_end_name);
+ list = tree_cons (ident, NULL_TREE, list);
}
tree clauses = NULL_TREE;
@@ -1547,6 +1548,7 @@ add_attributes_to_decl (symbol_attribute sym_attr, tree list)
clauses = c;
}
+ bool has_declare = true;
if (sym_attr.omp_declare_target_link
|| sym_attr.oacc_declare_link)
list = tree_cons (get_identifier ("omp declare target link"),
@@ -1558,12 +1560,45 @@ add_attributes_to_decl (symbol_attribute sym_attr, tree list)
|| sym_attr.oacc_declare_device_resident)
list = tree_cons (get_identifier ("omp declare target"),
clauses, list);
+ else
+ has_declare = false;
if (sym_attr.omp_declare_target_indirect)
list = tree_cons (get_identifier ("omp declare target indirect"),
clauses, list);
- return list;
+ decl_attributes (decl_p, list, 0);
+
+ if (has_declare
+ && VAR_P (*decl_p)
+ && sym->ns->proc_name->attr.flavor != FL_MODULE)
+ {
+ has_declare = false;
+ for (gfc_namespace* ns = sym->ns->contained; ns; ns = ns->sibling)
+ if (ns->proc_name->attr.omp_declare_target)
+ {
+ has_declare = true;
+ break;
+ }
+ }
+
+ if (has_declare && VAR_P (*decl_p) && has_declare)
+ {
+ /* Add to offload_vars; get_create does so for omp_declare_target,
+ omp_declare_target_link requires manual work. */
+ gcc_assert (symtab_node::get (*decl_p) == 0);
+ symtab_node *node = symtab_node::get_create (*decl_p);
+ if (node != NULL && sym_attr.omp_declare_target_link)
+ {
+ node->offloadable = 1;
+ if (ENABLE_OFFLOADING)
+ {
+ g->have_offload = true;
+ if (is_a <varpool_node *> (node))
+ vec_safe_push (offload_vars, *decl_p);
+ }
+ }
+ }
}
@@ -1578,7 +1613,6 @@ gfc_get_symbol_decl (gfc_symbol * sym)
{
tree decl;
tree length = NULL_TREE;
- tree attributes;
int byref;
bool intrinsic_array_parameter = false;
bool fun_or_res;
@@ -1864,12 +1898,6 @@ gfc_get_symbol_decl (gfc_symbol * sym)
decl = build_decl (gfc_get_location (&sym->declared_at),
VAR_DECL, gfc_sym_identifier (sym), gfc_sym_type (sym));
- /* Add attributes to variables. Functions are handled elsewhere. */
- attributes = add_attributes_to_decl (sym->attr, NULL_TREE);
- decl_attributes (&decl, attributes, 0);
- if (sym->ts.deferred && VAR_P (length))
- decl_attributes (&length, attributes, 0);
-
/* Symbols from modules should have their assembler names mangled.
This is done here rather than in gfc_finish_var_decl because it
is different for string length variables. */
@@ -2035,6 +2063,12 @@ gfc_get_symbol_decl (gfc_symbol * sym)
TREE_READONLY (decl) = 1;
}
+ /* Add attributes to variables. Functions are handled elsewhere. */
+ add_attributes_to_decl (&decl, sym);
+
+ if (sym->ts.deferred && VAR_P (length))
+ decl_attributes (&length, DECL_ATTRIBUTES (decl), 0);
+
return decl;
}
@@ -2071,7 +2105,6 @@ static tree
get_proc_pointer_decl (gfc_symbol *sym)
{
tree decl;
- tree attributes;
if (sym->module || sym->fn_result_spec)
{
@@ -2151,8 +2184,7 @@ get_proc_pointer_decl (gfc_symbol *sym)
&& (TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
set_decl_tls_model (decl, decl_default_tls_model (decl));
- attributes = add_attributes_to_decl (sym->attr, NULL_TREE);
- decl_attributes (&decl, attributes, 0);
+ add_attributes_to_decl (&decl, sym);
return decl;
}
@@ -2166,7 +2198,6 @@ gfc_get_extern_function_decl (gfc_symbol * sym, gfc_actual_arglist *actual_args,
{
tree type;
tree fndecl;
- tree attributes;
gfc_expr e;
gfc_intrinsic_sym *isym;
gfc_expr argexpr;
@@ -2364,8 +2395,7 @@ module_sym:
DECL_EXTERNAL (fndecl) = 1;
TREE_PUBLIC (fndecl) = 1;
- attributes = add_attributes_to_decl (sym->attr, NULL_TREE);
- decl_attributes (&fndecl, attributes, 0);
+ add_attributes_to_decl (&fndecl, sym);
gfc_set_decl_assembler_name (fndecl, mangled_name);
@@ -2424,7 +2454,7 @@ module_sym:
static void
build_function_decl (gfc_symbol * sym, bool global)
{
- tree fndecl, type, attributes;
+ tree fndecl, type;
symbol_attribute attr;
tree result_decl;
gfc_formal_arglist *f;
@@ -2475,15 +2505,14 @@ build_function_decl (gfc_symbol * sym, bool global)
if (sym->attr.referenced || sym->attr.entry_master)
TREE_USED (fndecl) = 1;
- attributes = add_attributes_to_decl (attr, NULL_TREE);
- decl_attributes (&fndecl, attributes, 0);
+ add_attributes_to_decl (&fndecl, sym);
/* Figure out the return type of the declared function, and build a
RESULT_DECL for it. If this is a subroutine with alternate
returns, build a RESULT_DECL for it. */
result_decl = NULL_TREE;
/* TODO: Shouldn't this just be TREE_TYPE (TREE_TYPE (fndecl)). */
- if (attr.function)
+ if (sym->attr.function)
{
if (gfc_return_by_reference (sym))
type = void_type_node;
@@ -2530,7 +2559,7 @@ build_function_decl (gfc_symbol * sym, bool global)
/* Set attributes for PURE functions. A call to a PURE function in the
Fortran 95 sense is both pure and without side effects in the C
sense. */
- if (attr.pure || attr.implicit_pure)
+ if (sym->attr.pure || sym->attr.implicit_pure)
{
/* TODO: check if a pure SUBROUTINE has no INTENT(OUT) arguments
including an alternate return. In that case it can also be
diff --git a/libgomp/testsuite/libgomp.fortran/declare-target-link.f90 b/libgomp/testsuite/libgomp.fortran/declare-target-link.f90
new file mode 100644
index 00000000000..2ce212d114f
--- /dev/null
+++ b/libgomp/testsuite/libgomp.fortran/declare-target-link.f90
@@ -0,0 +1,116 @@
+! { dg-additional-options "-Wall" }
+! PR fortran/115559
+
+module m
+ integer :: A
+ !$omp declare target link(A)
+end module m
+
+subroutine f
+ implicit none (type, external)
+ integer, save :: x, y ! { dg-warning "Unused variable 'y' declared" }
+ !$omp declare target link(x, y)
+
+ ! note: y is not 'link' as gfortran doesn't regard it as used
+ x = 6
+ call ii
+
+contains
+ subroutine k
+ !$omp declare target
+ use m
+ A = 5
+ end
+ subroutine ii
+ integer :: res
+ !$omp target map(x) map(from: res)
+ call k()
+ call ll()
+ res = get()
+ !$omp end target
+ ! print *, res
+ if (res /= 6 + 7 + 5) &
+ stop 1
+ end
+ subroutine ll
+ !$omp declare target
+ x = x + 7
+ end
+ integer function get()
+ use m
+ !$omp declare target
+ get = x + A
+ end
+end
+
+
+subroutine sub
+ implicit none (type, external)
+ integer, save :: arr(100), arr2(1:4)
+ !$omp declare target link(arr,arr2)
+
+ call mapit
+ call device1
+ call re_mapit
+ call device2
+contains
+ subroutine mapit
+ integer :: i
+ arr = [(i, i=1,100)]
+ !$omp target enter data map(to:arr(10:50)) map(alloc: arr2(1:4))
+ end subroutine
+ subroutine re_mapit
+ integer :: i
+ !$omp target exit data map(from:arr(10:50)) map(delete: arr2)
+
+ if (any (arr(1:9) /= [(i, i=1,9)])) stop 2
+ if (any (arr(10:50) /= [(3-10*i, i=10,50)])) stop 3
+ if (any (arr(51:100) /= [(i, i=51,100)])) stop 4
+ end subroutine
+
+ subroutine device1
+ integer :: res
+ !$omp target map(from:res)
+ res = run_device1()
+ !$omp end target
+ print *, res
+ ! FIXME: arr2 not link mapped -> PR115637
+ ! if (res /= -11436) stop 5
+ if (res /= -11546) stop 5 ! FIXME
+ end
+ integer function run_device1()
+ !$omp declare target
+ integer :: i
+ run_device1 = -99
+ ! FIXME: arr2 not link mapped -> PR115637
+ ! arr2 = [11,22,33,44]
+ if (any (arr(10:50) /= [(i, i=10,50)])) then
+ run_device1 = arr(11)
+ return
+ end if
+ ! FIXME: -> PR115637
+ ! run_device1 = sum(arr(10:13) + arr2)
+ run_device1 = sum(arr(10:13) ) ! FIXME
+ do i = 10, 50
+ arr(i) = 3 - 10 * arr(i)
+ end do
+ run_device1 = run_device1 + sum(arr(15:50))
+ end
+ subroutine device2
+ end
+ integer function run_device2()
+ !$omp declare target
+ run_device2 = -99
+ end
+end
+
+
+use m
+implicit none (type, external)
+external f
+external sub
+
+!$omp target enter data map(alloc: A)
+call f()
+call sub
+end
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch, v2] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR11555]
2024-07-26 18:34 ` [Patch, v2] " Tobias Burnus
@ 2024-07-29 7:09 ` Andre Vehreschild
2024-07-29 7:29 ` Tobias Burnus
2024-07-30 8:37 ` [committed] gfortran.dg/compiler-directive_2.f: Update dg-error (was: [Patch, v2] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR115559]) Tobias Burnus
1 sibling, 1 reply; 8+ messages in thread
From: Andre Vehreschild @ 2024-07-29 7:09 UTC (permalink / raw)
To: Tobias Burnus; +Cc: gcc-patches, Jakub Jelinek, fortran
Hi Tobias,
I am wondering why the testcase has no `!{ dg-do ... }` line. What will dejagnu
do then? Sorry for the may be stupid question, but I never encountered a
testcase without a dg-do line. It was the minimum for me.
Besides that the patch looks ok to me.
- Andre
On Fri, 26 Jul 2024 20:34:18 +0200
Tobias Burnus <tburnus@baylibre.com> wrote:
> Updated patch - only change is to the testcase:
>
> * With the just posted patch for PR116107, array sections with offset
> work for 'link', hence, I updated the testcase.
>
> * For 'arr2', I added ref to the associated PR.
>
> I intent to commit it once PR116107 has been committed.
>
> Tobias
>
> Tobias Burnus wrote:
> > Hi all,
> >
> > it turned out that 'declare target' with 'link' clause was broken in
> > multiple ways.
> >
> > The main fix is the attached patch, i.e. namely pushing the variables
> > already to the offload-vars list already in the FE.
> >
> > When implementing it, I noticed:
> > * C has a similar issue when using nested functions, which is
> > a GNU extension →https://gcc.gnu.org/115574
> >
> > * When doing partial mapping of arrays (which is one of the reasons for
> > 'link'), offsets are mishandled in Fortran (not tested in C), see FIXME in
> > the patch) There: arr2(10) should print 10 but with map(arr2(10:)) it
> > prints 19. (I will file a PR about this).
> >
> > * It might happen that linked variables do not get linked. I have not
> > investigated why, but 'arr2' gives link errors – while 'arr' works.
> > See FIXME in the patch. (I will file a PR about this)
> >
> > * For COMMON blocks, map(/common/) is rejected,https://gcc.gnu.org/PR115577
> >
> > * When then mapping map(a,b,c) which is identical for 'common /mycom/
> > a,b,c', it fails to link the device side as the 'mycom_' symbol cannot be
> > found on the device side. (I will file a PR about this)
> >
> > As COMMON as issues, an alternative would be to defer the trans-common.cc
> > changes to a later patch.
> >
> > Comments, questions, concerns?
> >
> > Tobias
> >
> > PS: Tested with nvptx offloading with a page-migration supporting system
> > with nvptx and GCN offloading configured and no new fails observed.
--
Andre Vehreschild * Email: vehre ad gmx dot de
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch, v2] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR11555]
2024-07-29 7:09 ` Andre Vehreschild
@ 2024-07-29 7:29 ` Tobias Burnus
2024-07-29 7:39 ` Andre Vehreschild
0 siblings, 1 reply; 8+ messages in thread
From: Tobias Burnus @ 2024-07-29 7:29 UTC (permalink / raw)
To: Andre Vehreschild, Tobias Burnus; +Cc: gcc-patches, Jakub Jelinek, fortran
Hi Andre,
Andre Vehreschild wrote:
> I am wondering why the testcase has no `!{ dg-do ... }` line. What will dejagnu
> do then? Sorry for the may be stupid question, but I never encountered a
> testcase without a dg-do line. It was the minimum for me.
Well, then you need look harder ;-)
In gcc/testsuite/, the default is '{ dg-do compile }', i.e. you can
specify or leave out that line without any additional effect. Having it
might be a tad clearer, albeit makes the test a tad longer.
But if you want to 'run' or 'link', you need to specify the dg-do line.
There are several files which don't have the "dg-do compile" line, also
under gcc/testsuite/gfortran.dg
In case of libgomp, it is becomes interesting: the default is running
the code, i.e. you need a 'compile' or 'link' when it shouldn't be run.
However, at least for Fortran (libgomp.{oacc-}fortran), there is a
difference between specifying nothing and specifying 'dg-do run': In
case of the default, it is compiled and run. But if you specify 'dg-do
run', it is compiled multiple times with different optimization options
and then run.
(Actually, also under gcc/testsuite/gfortran.dg, you get multiple
compilations + runs with 'dg-do run'. If you use dg-additional-options,
you can also add options. I think with dg-options, you set it to a
single run [not confirmed].)
The downside of compiling + running it multiple times is a longer test
time without any real benefit. However, especially with Fortran,
compiling with different optimization levels did expose issues in the
past, both in the Fortran front end and in the middle end. — Thus, there
some benefit of using it.
In any case, there more complex the code is that front-end + middle-end
code have to process, the more useful is "dg-do run". The more work is
done by the run-time library, be it libgfortran or libgomp, the less
useful it becomes as the heavy lifting is done in the run-time library.
— As libgomp progressing already takes quite some time (albeit it can
now run in parallel), there are some who prefer few 'dg-do run' and
others who prefer if all Fortran testcases there use 'dg-do run' …
I hope it helps,
Tobias
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch, v2] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR11555]
2024-07-29 7:29 ` Tobias Burnus
@ 2024-07-29 7:39 ` Andre Vehreschild
2024-07-29 7:53 ` Tobias Burnus
0 siblings, 1 reply; 8+ messages in thread
From: Andre Vehreschild @ 2024-07-29 7:39 UTC (permalink / raw)
To: Tobias Burnus; +Cc: Tobias Burnus, gcc-patches, Jakub Jelinek, fortran
Thanks a lot Tobias,
yes, I could have looked harder :-)
This isn't by any chance documented on the developer website of gcc somewhere?
It would be sad, if that knowledge is not publicy available for the future.
Thanks again for the explanation and keep up the good work.
Regards,
Andre
On Mon, 29 Jul 2024 09:29:28 +0200
Tobias Burnus <burnus@net-b.de> wrote:
> Hi Andre,
>
> Andre Vehreschild wrote:
> > I am wondering why the testcase has no `!{ dg-do ... }` line. What will
> > dejagnu do then? Sorry for the may be stupid question, but I never
> > encountered a testcase without a dg-do line. It was the minimum for me.
>
> Well, then you need look harder ;-)
>
> In gcc/testsuite/, the default is '{ dg-do compile }', i.e. you can
> specify or leave out that line without any additional effect. Having it
> might be a tad clearer, albeit makes the test a tad longer.
>
> But if you want to 'run' or 'link', you need to specify the dg-do line.
> There are several files which don't have the "dg-do compile" line, also
> under gcc/testsuite/gfortran.dg
>
> In case of libgomp, it is becomes interesting: the default is running
> the code, i.e. you need a 'compile' or 'link' when it shouldn't be run.
>
> However, at least for Fortran (libgomp.{oacc-}fortran), there is a
> difference between specifying nothing and specifying 'dg-do run': In
> case of the default, it is compiled and run. But if you specify 'dg-do
> run', it is compiled multiple times with different optimization options
> and then run.
>
> (Actually, also under gcc/testsuite/gfortran.dg, you get multiple
> compilations + runs with 'dg-do run'. If you use dg-additional-options,
> you can also add options. I think with dg-options, you set it to a
> single run [not confirmed].)
>
> The downside of compiling + running it multiple times is a longer test
> time without any real benefit. However, especially with Fortran,
> compiling with different optimization levels did expose issues in the
> past, both in the Fortran front end and in the middle end. — Thus, there
> some benefit of using it.
>
> In any case, there more complex the code is that front-end + middle-end
> code have to process, the more useful is "dg-do run". The more work is
> done by the run-time library, be it libgfortran or libgomp, the less
> useful it becomes as the heavy lifting is done in the run-time library.
> — As libgomp progressing already takes quite some time (albeit it can
> now run in parallel), there are some who prefer few 'dg-do run' and
> others who prefer if all Fortran testcases there use 'dg-do run' …
>
> I hope it helps,
>
> Tobias
>
--
Andre Vehreschild * Email: vehre ad gmx dot de
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch, v2] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR11555]
2024-07-29 7:39 ` Andre Vehreschild
@ 2024-07-29 7:53 ` Tobias Burnus
2024-07-29 8:22 ` Jakub Jelinek
0 siblings, 1 reply; 8+ messages in thread
From: Tobias Burnus @ 2024-07-29 7:53 UTC (permalink / raw)
To: Andre Vehreschild, Tobias Burnus; +Cc: gcc-patches, Jakub Jelinek, fortran
[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]
Hi Andre, hi all,
Andre Vehreschild wrote:
> yes, I could have looked harder 🙂
I wrote ;-) on purpose as this feature is somewhat hidden and writing
'dg-do compile' doesn't harm.
In case of gcc/testsuite, the 'run' is also needed and were often missed
(or rather caused by invalid variants such as 'dg-run' (should be:
'dg-do run') or '{dg-do run }' (missing space after '{') prevented the
running of the code). Sam did fix some of those (and some other dg-*
issues) recently, e.g. in r15-2349-ga75c6295252d0d (→
https://gcc.gnu.org/r15-2349-ga75c6295252d0d ).
> This isn't by any chance documented on the developer website of gcc somewhere?
> It would be sad, if that knowledge is not publicy available for the future.
https://gcc.gnu.org/onlinedocs/gccint/Directives.html#Specify-how-to-build-the-test
documents it.
And libgomp has: lib/libgomp.exp:set dg-do-what-default run
The all arguments vs. only -O2 is set in libgomp via:
libgomp.c++/c++.exp: set DEFAULT_CFLAGS "-O2"
libgomp.c/c.exp: set DEFAULT_CFLAGS "-O2"
and for libgomp.*fortran/fortran.exp, the difference between 'dg-do run'
vs. default is *not* *documented,* but seems to be the result of the
following:
# For Fortran we're doing torture testing, as Fortran has far more tests
# with arrays etc. that testing just -O0 or -O2 is insufficient, that is
# typically not the case for C/C++.
gfortran-dg-runtest $tests "" ""
Tobias
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch, v2] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR11555]
2024-07-29 7:53 ` Tobias Burnus
@ 2024-07-29 8:22 ` Jakub Jelinek
0 siblings, 0 replies; 8+ messages in thread
From: Jakub Jelinek @ 2024-07-29 8:22 UTC (permalink / raw)
To: Tobias Burnus; +Cc: Andre Vehreschild, Tobias Burnus, gcc-patches, fortran
On Mon, Jul 29, 2024 at 09:53:47AM +0200, Tobias Burnus wrote:
> Hi Andre, hi all,
>
> Andre Vehreschild wrote:
> > yes, I could have looked harder 🙂
>
> I wrote ;-) on purpose as this feature is somewhat hidden and writing 'dg-do
> compile' doesn't harm.
I think an explicit dg-do is better, otherwise one has to just guess
for some tests what has been actually intentional (see the recent
torture tests which were just compile time but written most likely to be
runtime; I've changed a few, Sam changed more).
Also, the subject line has too few digits in the PR number I think (9
missing?).
Otherwise LGTM.
Jakub
^ permalink raw reply [flat|nested] 8+ messages in thread
* [committed] gfortran.dg/compiler-directive_2.f: Update dg-error (was: [Patch, v2] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR115559])
2024-07-26 18:34 ` [Patch, v2] " Tobias Burnus
2024-07-29 7:09 ` Andre Vehreschild
@ 2024-07-30 8:37 ` Tobias Burnus
1 sibling, 0 replies; 8+ messages in thread
From: Tobias Burnus @ 2024-07-30 8:37 UTC (permalink / raw)
To: gcc-patches, fortran; +Cc: Jiang, Haochen
Follow up fix:
As the !GCC$ attributes are now added in reverse order,
the 'stdcall' vs. 'fastcall' in the error message swapped order:
"Error: stdcall and fastcall attributes are not compatible" This didn't
show up here with -m64 ("Warning: 'stdcall' attribute ignored") and I
didn't run it with -m32, but it was reported by Haochen's script +
manually confirmed by him.
(Thanks for the report and checking – and sorry for the FAIL.)
Committed asr15-2401-g15158a8853a69f. Tobias
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-07-30 8:37 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-06-21 13:06 [Patch] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR11555] Tobias Burnus
2024-07-26 18:34 ` [Patch, v2] " Tobias Burnus
2024-07-29 7:09 ` Andre Vehreschild
2024-07-29 7:29 ` Tobias Burnus
2024-07-29 7:39 ` Andre Vehreschild
2024-07-29 7:53 ` Tobias Burnus
2024-07-29 8:22 ` Jakub Jelinek
2024-07-30 8:37 ` [committed] gfortran.dg/compiler-directive_2.f: Update dg-error (was: [Patch, v2] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR115559]) Tobias Burnus
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).