On Mon, Oct 14, 2019 at 03:11:43PM +0200, Tobias Burnus wrote:
> gcc/fortran/
> * f95-lang.c (LANG_HOOKS_OMP_ARRAY_DATA): Set to gfc_omp_array_data.
> * trans-array.c
Description of what has been changed and how is missing.
> --- a/gcc/langhooks.h
> +++ b/gcc/langhooks.h
> @@ -222,6 +222,10 @@ struct lang_hooks_for_decls
> /* True if this decl may be called via a sibcall. */
> bool (*ok_for_sibcall) (const_tree);
>
> + /* Return a tree for of the actual data of an array descriptor - or
s/for of/for/ ?
> + NULL_TREE if original tree is not an array descriptor. */
> + tree (*omp_array_data) (tree);
> +
> --- a/gcc/omp-low.c
> +++ b/gcc/omp-low.c
> @@ -90,6 +90,7 @@ struct omp_context
> /* Map variables to fields in a structure that allows communication
> between sending and receiving threads. */
> splay_tree field_map;
> + splay_tree array_data_map;
I was hoping you could get away without introducing yet another splay_tree
for this. As I said on IRC, we already do have the need to record two
different fields for the same decl and do that through using
the decl address (var) once as the splay_tree_key and in another case
as &DECL_UID (var). So can't you use the mask & 16 mode to use say
&DECL_NAME (var) and pass the decl to install_var_field rather than x?
> - if ((mask & 8) != 0)
> + if ((mask & 16) != 0)
> + key = (splay_tree_key) var;
That would be here.
> + else if ((mask & 8) != 0)
> {
> key = (splay_tree_key) &DECL_UID (var);
> gcc_checking_assert (key != (splay_tree_key) var);
> @@ -745,14 +748,17 @@ install_var_field (tree var, bool by_ref, int mask,
> omp_context *ctx)
> else if ((mask & 3) == 1 && omp_is_reference (var))
> type = TREE_TYPE (type);
>
> - field = build_decl (DECL_SOURCE_LOCATION (var),
> - FIELD_DECL, DECL_NAME (var), type);
> + if ((mask & 16) != 0)
> + field = build_decl (UNKNOWN_LOCATION, FIELD_DECL, NULL_TREE, type);
> + else
> + field = build_decl (DECL_SOURCE_LOCATION (var),
> + FIELD_DECL, DECL_NAME (var), type);
And you could use the DECL_NAME as well as DECL_SOURCE_LOCATION.
It is true that for mode & 16 it would need to use the target hook to
actually find out the type of the field, so perhaps we'd need to call
the new target hook multiple times. As it creates trees that is not the
best idea, though perhaps it could have an extra bool argument where it
would return the type of the data pointer in the descriptor (+ info whether
it is a descriptor through whether it returns non-NULL) with false for the
new arg, something that would be cheap to be called multiple times, once in
scan_sharing_clauses, then again in install_var_field for mode & 16, and
with true in the hopefully only spot where we need the actual expression
(where we gimplify it and store into the corresponding field).
> @@ -1070,7 +1078,7 @@ fixup_child_record_type (omp_context *ctx)
> static void
> scan_sharing_clauses (tree clauses, omp_context *ctx)
> {
> - tree c, decl;
> + tree c, decl, x;
> bool scan_array_reductions = false;
>
> for (c = clauses; c; c = OMP_CLAUSE_CHAIN (c))
> @@ -1240,10 +1248,33 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
> case OMP_CLAUSE_USE_DEVICE_PTR:
> case OMP_CLAUSE_USE_DEVICE_ADDR:
> decl = OMP_CLAUSE_DECL (c);
> - if ((OMP_CLAUSE_CODE (c) == OMP_CLAUSE_USE_DEVICE_ADDR
> - && !omp_is_reference (decl)
> - && !omp_is_allocatable_or_ptr (decl))
> - || TREE_CODE (TREE_TYPE (decl)) == ARRAY_TYPE)
> + x = NULL;
> + // Handle array descriptors
Most of omp-low.c uses /* ... */ comments, can you use them here too
(plus dot at the end + two spaces before */)?
> + if (TREE_CODE (TREE_TYPE (decl)) == RECORD_TYPE ||
Formatting, || needs to be on the following line.
> + (omp_is_reference (decl)
> + && TREE_CODE (TREE_TYPE (TREE_TYPE (decl))) == RECORD_TYPE))
> + x = lang_hooks.decls.omp_array_data (decl);
> +
> + if (x)
> + {
> + gcc_assert (!ctx->array_data_map
> + || !splay_tree_lookup (ctx->array_data_map,
> + (splay_tree_key) decl));
> + if (!ctx->array_data_map)
> + ctx->array_data_map
> + = splay_tree_new (splay_tree_compare_pointers, 0, 0);
Formatting, indented too much (though, see above, I'd hope you can avoid
that).
> +
> + splay_tree_insert (ctx->array_data_map, (splay_tree_key) decl,
> + (splay_tree_value) x);
> +
> + install_var_field (x, false, 19, ctx);
> + DECL_SOURCE_LOCATION (lookup_field (x, ctx))
> + = OMP_CLAUSE_LOCATION (c);
Formatting.
> - if (OMP_CLAUSE_CODE (c) != OMP_CLAUSE_IS_DEVICE_PTR)
> +
> + // For arrays with descriptor, use the pointer to the actual data
Comment style.
> + tkind = OMP_CLAUSE_CODE (c) != OMP_CLAUSE_IS_DEVICE_PTR
> + ? GOMP_MAP_USE_DEVICE_PTR : GOMP_MAP_FIRSTPRIVATE_INT;
I think emacs users (I'm not one of them) prefer here
tkind = (OMP_...
? ...);
Jakub
