Re: [Patch] Fortran: Support OpenMP's 'allocate' directive for stack vars
On Wed, Oct 18, 2023 at 11:12:44AM +0200, Thomas Schwinge wrote: > Hi Tobias! > > On 2023-10-13T15:29:52+0200, Tobias Burnus wrote: > > => Updated patch attached > > When cherry-picking this commit 2d3dbf0eff668bed5f5f168b3cafd8590c54 > "Fortran: Support OpenMP's 'allocate' directive for stack vars" on top of > slightly older GCC sources (mentioning that just in case that's > relevant), in a configuration with offloading enabled (only), I see: > > +FAIL: gfortran.dg/gomp/allocate-13.f90 -O (internal compiler error: > tree code 'statement_list' is not supported in LTO streams) > +FAIL: gfortran.dg/gomp/allocate-13.f90 -O (test for excess errors) Any references to GENERIC code in clauses etc. should have been gimplified or cleared during gimplification, we shouldn't support STATEMENT_LIST in LTO streaming. Jakub
Re: [Patch] Fortran: Support OpenMP's 'allocate' directive for stack vars
Hi Tobias! On 2023-10-13T15:29:52+0200, Tobias Burnus wrote: > => Updated patch attached When cherry-picking this commit 2d3dbf0eff668bed5f5f168b3cafd8590c54 "Fortran: Support OpenMP's 'allocate' directive for stack vars" on top of slightly older GCC sources (mentioning that just in case that's relevant), in a configuration with offloading enabled (only), I see: +FAIL: gfortran.dg/gomp/allocate-13.f90 -O (internal compiler error: tree code 'statement_list' is not supported in LTO streams) +FAIL: gfortran.dg/gomp/allocate-13.f90 -O (test for excess errors) during IPA pass: modref [...]/gcc/testsuite/gfortran.dg/gomp/allocate-13.f90:10:3: internal compiler error: tree code 'statement_list' is not supported in LTO streams 0x13374fd lto_write_tree [...]/gcc/lto-streamer-out.cc:561 0x13374fd lto_output_tree_1 [...]/gcc/lto-streamer-out.cc:599 0x133f55b DFS::DFS(output_block*, tree_node*, bool, bool, bool) [...]/gcc/lto-streamer-out.cc:899 0x1340287 lto_output_tree(output_block*, tree_node*, bool, bool) [...]/gcc/lto-streamer-out.cc:1865 0x134197a output_function [...]/gcc/lto-streamer-out.cc:2436 0x134197a lto_output() [...]/gcc/lto-streamer-out.cc:2807 0x13d0551 write_lto [...]/gcc/passes.cc:2774 0x13d0551 ipa_write_summaries_1 [...]/gcc/passes.cc:2838 0x13d0551 ipa_write_summaries() [...]/gcc/passes.cc:2894 0x1002f2c ipa_passes [...]/gcc/cgraphunit.cc:2251 0x1002f2c symbol_table::compile() [...]/gcc/cgraphunit.cc:2331 0x10056b7 symbol_table::compile() [...]/gcc/cgraphunit.cc:2311 0x10056b7 symbol_table::finalize_compilation_unit() [...]/gcc/cgraphunit.cc:2583 Similarly: +FAIL: libgomp.fortran/allocate-6.f90 -O (internal compiler error: tree code 'statement_list' is not supported in LTO streams) +FAIL: libgomp.fortran/allocate-7.f90 -O (internal compiler error: tree code 'statement_list' is not supported in LTO streams) Grüße Thomas > Fortran: Support OpenMP's 'allocate' directive for stack vars > > gcc/fortran/ChangeLog: > > * gfortran.h (ext_attr_t): Add omp_allocate flag. > * match.cc (gfc_free_omp_namelist): Void deleting same > u2.allocator multiple times now that a sequence can use > the same one. > * openmp.cc (gfc_match_omp_clauses, gfc_match_omp_allocate): Use > same allocator expr multiple times. > (is_predefined_allocator): Make static. > (gfc_resolve_omp_allocate): Update/extend restriction checks; > remove sorry message. > (resolve_omp_clauses): Reject corarrays in allocate/allocators > directive. > * parse.cc (check_omp_allocate_stmt): Permit procedure pointers > here (rejected later) for less misleading diagnostic. > * trans-array.cc (gfc_trans_auto_array_allocation): Propagate > size for GOMP_alloc and location to which it should be added to. > * trans-decl.cc (gfc_trans_deferred_vars): Handle 'omp allocate' > for stack variables; sorry for static variables/common blocks. > * trans-openmp.cc (gfc_trans_omp_clauses): Evaluate 'allocate' > clause's allocator only once; fix adding expressions to the > block. > (gfc_trans_omp_single): Pass a block to gfc_trans_omp_clauses. > > gcc/ChangeLog: > > * gimplify.cc (gimplify_bind_expr): Handle Fortran's > 'omp allocate' for stack variables. > > libgomp/ChangeLog: > > * libgomp.texi (OpenMP Impl. Status): Mention that Fortran now > supports the allocate directive for stack variables. > * testsuite/libgomp.fortran/allocate-5.f90: New test. > * testsuite/libgomp.fortran/allocate-6.f90: New test. > * testsuite/libgomp.fortran/allocate-7.f90: New test. > * testsuite/libgomp.fortran/allocate-8.f90: New test. > > gcc/testsuite/ChangeLog: > > * c-c++-common/gomp/allocate-14.c: Fix directive name. > * c-c++-common/gomp/allocate-15.c: Likewise. > * c-c++-common/gomp/allocate-9.c: Fix comment typo. > * gfortran.dg/gomp/allocate-4.f90: Remove sorry dg-error. > * gfortran.dg/gomp/allocate-7.f90: Likewise. > * gfortran.dg/gomp/allocate-10.f90: New test. > * gfortran.dg/gomp/allocate-11.f90: New test. > * gfortran.dg/gomp/allocate-12.f90: New test. > * gfortran.dg/gomp/allocate-13.f90: New test. > * gfortran.dg/gomp/allocate-14.f90: New test. > * gfortran.dg/gomp/allocate-15.f90: New test. > * gfortran.dg/gomp/allocate-8.f90: New test. > * gfortran.dg/gomp/allocate-9.f90: New test. > > gcc/fortran/gfortran.h | 1 + > gcc/fortran/match.cc | 9 +- > gcc/fortran/openmp.cc| 62 +++- > gcc/fortran/parse.cc | 8 +- > gcc/fortran/trans-array.cc
Re: [Patch] Fortran: Support OpenMP's 'allocate' directive for stack vars
On 13.10.23 13:01, Jakub Jelinek wrote:
On Tue, Oct 10, 2023 at 06:46:35PM +0200, Tobias Burnus wrote:
+++ b/gcc/gimplify.cc
@@ -1400,23 +1400,53 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
+ else if (errorcount
+ || (align == NULL_TREE
+ && alloc != NULL_TREE
+ && integer_onep (alloc)))
+DECL_ATTRIBUTES (t) = remove_attribute ("omp allocate",
+DECL_ATTRIBUTES (t));
Probably already preexisting, by I wonder how safe remove_attribute is.
Aren't attributes shared in some cases
(like __attribute__((attr1, attr2, attr3)) int a, b, c, d;)?
I think there should be no problem – new attributes get added as:
DECL_ATTRIBUTES (var) = tree_cons (get_identifier ("omp allocate"),
t, DECL_ATTRIBUTES (var));
Thus, attr = DECL_ATTRIBUTES (var) is not shared, even if
TREE_CHAIN (attr) might be shared.
Thus, as long as new attributes get added at the head of the chain,
there should be no issue. And "omp allocate" is added once per decl
and is therefore not shared - removing might create again a shared
tree, but any following 'tree_cons' will again unshare it.
I think in your case, there would be indeed a problem when doing:
'tree attr = remove_attr (...("attr2") ...)'
as this would remove "attr2" for 4 decls.
* * *
However, as 'omp allocate' is not used later on, I also do not need
to remove it.
=> Updated patch attached + interdiff for gimplify.cc attached.
Changes:
* Condition now the same as previously
* Keep 'omp allocate' also for DECL_VALUE_EXPR variable
* Add assert that we either have Fortran's expression the GOMP_FREE
location - or a DECL_VALUE_EXPR.
* Make use of the assertion by keeping the HAS_DECL_VALUE expr below;
this avoids adding an align/allocator == default check.
=> same code as old code, except for creating + using 'v' variable
and adding a clobber.
* * *
Otherwise LGTM (though, I didn't spot anything about allocatables in the
patch, am I just looking wrong or are they still unsupported)?
It's unsupported – albeit Chung-Lin has some patch for it. The code path
is completely different. It already starts by 'omp allocators' (+
legacy: 'omp allocate') being not declarative but executable and
associated with a Fortran 'allocate' statement and ...
Sorry for being slow - but I keep getting distracted by other tasks ...
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
diff -u b/gcc/gimplify.cc b/gcc/gimplify.cc
--- b/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -1400,13 +1400,10 @@
"region must specify an % clause", t);
/* Skip for omp_default_mem_alloc (= 1),
unless align is present. */
- else if (errorcount
- || (align == NULL_TREE
- && alloc != NULL_TREE
- && integer_onep (alloc)))
- DECL_ATTRIBUTES (t) = remove_attribute ("omp allocate",
- DECL_ATTRIBUTES (t));
- else
+ else if (!errorcount
+ && (align != NULL_TREE
+ || alloc == NULL_TREE
+ || !integer_onep (alloc)))
{
/* Fortran might already use a pointer type internally;
use that pointer except for type(C_ptr) and type(C_funptr);
@@ -1426,10 +1423,10 @@
tmp = build_pointer_type (type);
v = create_tmp_var (tmp, get_name (t));
DECL_IGNORED_P (v) = 0;
- tmp = remove_attribute ("omp allocate", DECL_ATTRIBUTES (t));
DECL_ATTRIBUTES (v)
= tree_cons (get_identifier ("omp allocate var"),
- build_tree_list (NULL_TREE, t), tmp);
+ build_tree_list (NULL_TREE, t),
+ DECL_ATTRIBUTES (t));
tmp = build_fold_indirect_ref (v);
TREE_THIS_NOTRAP (tmp) = 1;
SET_DECL_VALUE_EXPR (t, tmp);
@@ -1461,6 +1458,11 @@
tmp = fold_build2_loc (loc, MODIFY_EXPR, TREE_TYPE (v), v,
fold_convert (TREE_TYPE (v), tmp));
gcc_assert (BIND_EXPR_BODY (bind_expr) != NULL_TREE);
+ /* Ensure that either TREE_CHAIN (TREE_VALUE (attr) is set
+ and GOMP_FREE added here or that DECL_HAS_VALUE_EXPR_P (t)
+ is set, using in a condition much further below. */
+ gcc_assert (DECL_HAS_VALUE_EXPR_P (t)
+ || TREE_CHAIN (TREE_VALUE (attr)));
if (TREE_CHAIN (TREE_VALUE (attr)))
{
/* Fortran is special as it does not have properly nest
@@ -1631,14 +1633,15 @@
{
tree attr;
if (flag_openmp
+ && DECL_HAS_VALUE_EXPR_P (t)
&& TREE_USED (t)
&& ((attr = lookup_attribute ("omp allocate",
DECL_ATTRIBUTES (t))) != NULL_TREE)
&& TREE_CHAIN (TREE_VALUE (attr)) == NULL_TREE)
{
- /* For Fortran, the GOMP_free has already been added above. */
- tree v = (DECL_HAS_VALUE_EXPR_P (t)
-
Re: [Patch] Fortran: Support OpenMP's 'allocate' directive for stack vars
On Tue, Oct 10, 2023 at 06:46:35PM +0200, Tobias Burnus wrote:
> * parse.cc (check_omp_allocate_stmt): Permit procedure pointers
> here (rejected later) for less mislreading diagnostic.
s/misl/mis/
> libgomp/ChangeLog:
>
> * libgomp.texi:
Fill in something here.
> @@ -7220,8 +7227,7 @@ gfc_resolve_omp_allocate (gfc_namespace *ns,
> gfc_omp_namelist *list)
>>where);
> continue;
> }
> - if (ns != n->sym->ns || n->sym->attr.use_assoc
> - || n->sym->attr.host_assoc || n->sym->attr.imported)
> + if (ns != n->sym->ns || n->sym->attr.use_assoc ||
> n->sym->attr.imported)
s/ n/ n/
> --- a/gcc/gimplify.cc
> +++ b/gcc/gimplify.cc
> @@ -1400,23 +1400,53 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
> "region must specify an % clause", t);
> /* Skip for omp_default_mem_alloc (= 1),
>unless align is present. */
> - else if (!errorcount
> -&& (align != NULL_TREE
> -|| alloc == NULL_TREE
> -|| !integer_onep (alloc)))
> + else if (errorcount
> +|| (align == NULL_TREE
> +&& alloc != NULL_TREE
> +&& integer_onep (alloc)))
> + DECL_ATTRIBUTES (t) = remove_attribute ("omp allocate",
> + DECL_ATTRIBUTES (t));
Probably already preexisting, by I wonder how safe remove_attribute is.
Aren't attributes shared in some cases
(like __attribute__((attr1, attr2, attr3)) int a, b, c, d;)?
Not really sure if something unshares them afterwards.
If they are shared, adding new attributes is fine, that will make the new
additions not shared and the tail shared, but remove_attribute could remove
it from all of them at once. Perhaps I'm wrong, haven't verified.
Otherwise LGTM (though, I didn't spot anything about allocatables in the
patch, am I just looking wrong or are they still unsupported)?
Jakub
