On Mon, Jun 03, 2019 at 05:02:45PM +0100, Julian Brown wrote:
> * omp-low.c (omp_context): Add oacc_partitioning_level and
> oacc_addressable_var_decls fields.
> (new_omp_context): Initialize oacc_addressable_var_decls in new
> omp_context.
> (delete_omp_context): Delete oacc_addressable_var_decls in old
> omp_context.
> (lower_oacc_head_tail): Record partitioning-level count in omp
> context.
> (oacc_record_private_var_clauses, oacc_record_vars_in_bind,
> mark_oacc_gangprivate): New functions.
> (lower_omp_for): Call oacc_record_private_var_clauses with "for"
> clauses.
> (lower_omp_target): Likewise, for "target" clauses.
> Call mark_oacc_gangprivate for offloaded target regions.
> (process_oacc_gangprivate_1): New function.
> (lower_omp_1): Call oacc_record_vars_in_bind for GIMPLE_BIND within
> OMP
> regions.
> (execute_lower_omp): Call process_oacc_gangprivate_1 for each OMP
> context.
Just commenting on the above part:
> --- a/gcc/omp-low.c
> +++ b/gcc/omp-low.c
> @@ -137,6 +137,12 @@ struct omp_context
>
> /* True if this construct can be cancelled. */
> bool cancellable;
> +
> + /* The number of levels of OpenACC partitioning invoked in this context.
> */
> + unsigned oacc_partitioning_levels;
> +
> + /* Addressable variable decls in this context. */
> + vec<tree> *oacc_addressable_var_decls;
Why vec<tree> * rather than vec<tree>?
> @@ -878,6 +884,7 @@ new_omp_context (gimple *stmt, omp_context *outer_ctx)
> }
>
> ctx->cb.decl_map = new hash_map<tree, tree>;
> + ctx->oacc_addressable_var_decls = new vec<tree> ();
You then don't have to new it here and delete below. As the context is
cleared with XCNEW, you don't need to do anything here, and just
release when deleting. Note, even if using a pointer for some reason was
needed (not in this case), using unconditional new for something only used
for small subset of contexts is unacceptable, it would be then desirable to
only create when needed.
>
> return ctx;
> }
> @@ -960,6 +967,7 @@ delete_omp_context (splay_tree_value value)
> }
>
> delete ctx->lastprivate_conditional_map;
> + delete ctx->oacc_addressable_var_decls;
>
> XDELETE (ctx);
> }
> @@ -8458,6 +8469,79 @@ lower_omp_for_lastprivate (struct omp_for_data *fd,
> gimple_seq *body_p,
> }
> }
>
> +/* Record vars listed in private clauses in CLAUSES in CTX. This information
> + is used to mark up variables that should be made private per-gang. */
> +
> +static void
> +oacc_record_private_var_clauses (omp_context *ctx, tree clauses)
> +{
> + tree c;
> +
> + if (!ctx)
> + return;
> +
> + for (c = clauses; c; c = OMP_CLAUSE_CHAIN (c))
> + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_PRIVATE)
> + {
> + tree decl = OMP_CLAUSE_DECL (c);
> + if (VAR_P (decl) && TREE_ADDRESSABLE (decl))
> + ctx->oacc_addressable_var_decls->safe_push (decl);
> + }
> +}
You don't want to do this for all GOMP_FOR or GOMP_TARGET context, I'd hope
you only want to do that for OpenACC contexts. Perhaps it is ok
to bail out early if the context isn't OpenACC one. On the other side, the
if (!ctx) condition makes no sense, the callers of course guarantee that ctx
is non-NULL.
> @@ -10665,6 +10774,7 @@ lower_omp_1 (gimple_stmt_iterator *gsi_p, omp_context
> *ctx)
> ctx);
> break;
> case GIMPLE_BIND:
> + oacc_record_vars_in_bind (ctx, gimple_bind_vars (as_a <gbind *>
> (stmt)));
Again, why is this done unconditionally? It should be relevant to gather it
only in some subset of context, so guard that and don't do it otherwise.
> lower_omp (gimple_bind_body_ptr (as_a <gbind *> (stmt)), ctx);
> maybe_remove_omp_member_access_dummy_vars (as_a <gbind *> (stmt));
> break;
> @@ -10905,6 +11015,7 @@ execute_lower_omp (void)
>
> if (all_contexts)
> {
> + splay_tree_foreach (all_contexts, process_oacc_gangprivate_1, NULL);
Similarly. Either guard with if (flag_openacc), or have some flag cleared
at the start of the pass and set only if you find something interesting so
that the splay_tree_foreach does something.
Jakub
