On Sat, Jul 20, 2024 at 02:42:21PM -0600, Sandra Loosemore wrote:
> --- a/gcc/cgraph.h
> +++ b/gcc/cgraph.h
> @@ -900,6 +900,7 @@ struct GTY((tag ("SYMTAB_FUNCTION"))) cgraph_node :
> public symtab_node
> ipcp_clone (false), declare_variant_alt (false),
> calls_declare_variant_alt (false), gc_candidate (false),
> called_by_ifunc_resolver (false),
> + has_metadirectives (false),
has_omp_metadirectives please. People unfamiliar with OpenMP will have no
idea what it is about.
> m_uid (uid), m_summary_id (-1)
> {}
>
> @@ -1501,6 +1502,8 @@ struct GTY((tag ("SYMTAB_FUNCTION"))) cgraph_node :
> public symtab_node
> unsigned gc_candidate : 1;
> /* Set if the function is called by an IFUNC resolver. */
> unsigned called_by_ifunc_resolver : 1;
> + /* True if the function contains unresolved metadirectives. */
And OpenMP before metadirectives in the comment.
> + unsigned has_metadirectives : 1;
> + unsigned i;
> +
> + /* The variants are not used after lowering. */
> + gimple_omp_metadirective_set_variants (stmt, NULL);
> +
> + for (i = 0; i < gimple_num_ops (stmt); i++)
Nothing uses i before or after the loop, so please do
for (unsigned i = 0; i < gimple_num_ops (stmt); i++)
> +gomp_variant *
> +gimple_build_omp_variant (gimple_seq body)
> +{
> + gomp_variant *variant = as_a <gomp_variant *>
> + (gimple_alloc (GIMPLE_OMP_METADIRECTIVE_VARIANT, 0));
gomp_variant *variant
= as_a <gomp_variant *> (gimple_alloc (GIMPLE_OMP_METADIRECTIVE_VARIANT,
0));
please instead.
> --- a/gcc/gimple.def
> +++ b/gcc/gimple.def
> @@ -398,6 +398,13 @@ DEFGSCODE(GIMPLE_OMP_TEAMS, "gimple_omp_teams",
> GSS_OMP_PARALLEL_LAYOUT)
> CLAUSES is an OMP_CLAUSE chain holding the associated clauses. */
> DEFGSCODE(GIMPLE_OMP_ORDERED, "gimple_omp_ordered", GSS_OMP_SINGLE_LAYOUT)
>
> +/* GIMPLE_OMP_METADIRECTIVE represents #pragma omp metadirective. */
> +DEFGSCODE(GIMPLE_OMP_METADIRECTIVE, "gimple_omp_metadirective",
> + GSS_OMP_METADIRECTIVE)
This one is misplaced.
> +
> +DEFGSCODE(GIMPLE_OMP_METADIRECTIVE_VARIANT,
> + "gimple_omp_variant", GSS_OMP_METADIRECTIVE_VARIANT)
> +
> /* GIMPLE_PREDICT <PREDICT, OUTCOME> specifies a hint for branch prediction.
>
> PREDICT is one of the predictors from predict.def.
> @@ -2149,7 +2209,8 @@ gimple_init_singleton (gimple *g)
> inline bool
> gimple_has_ops (const gimple *g)
> {
> - return gimple_code (g) >= GIMPLE_COND && gimple_code (g) <= GIMPLE_RETURN;
> + return (gimple_code (g) >= GIMPLE_COND && gimple_code (g) <= GIMPLE_RETURN)
> + || gimple_code (g) == GIMPLE_OMP_METADIRECTIVE;
> }
>
> template <>
As can be seen on the need to change gimple_has_ops. gimple_has_ops
shouldn't be changed, instead GIMPLE_OMP_DIRECTIVE should go before
GIMPLE_RETURN.
> + selectors.safe_push (selector);
> + gomp_variant *omp_variant
> + = gimple_build_omp_variant (NULL);
This surely fits on one line:
gomp_variant *omp_variant = gimple_build_omp_variant (NULL);
> @@ -10752,6 +10754,10 @@ build_omp_regions_1 (basic_block bb, struct
> omp_region *parent,
> /* GIMPLE_OMP_SECTIONS_SWITCH is part of
> GIMPLE_OMP_SECTIONS, and we do nothing for it. */
> }
> + else if (code == GIMPLE_OMP_METADIRECTIVE)
> + {
> + /* Do nothing for metadirectives. */
> + }
Better just change the following else
> else
/* Do nothing for metadirectives; otherwise: */
else if (code != GIMPLE_OMP_METADIRECTIVES)
> {
> region = new_omp_region (bb, code, parent);
> @@ -1003,6 +1008,18 @@ new_omp_context (gimple *stmt, omp_context *outer_ctx)
> return ctx;
> }
>
Add function comment here?
> +static omp_context *
> +clone_omp_context (omp_context *ctx)
> +{
> + omp_context *clone_ctx = XCNEW (omp_context);
If you memcpy over it all, XCNEW makes no sense, just XNEW?
> +
> + memcpy (clone_ctx, ctx, sizeof (omp_context));
> --- a/gcc/tree-cfg.cc
> +++ b/gcc/tree-cfg.cc
> @@ -1752,6 +1752,18 @@ cleanup_dead_labels (void)
> }
> break;
>
> + case GIMPLE_OMP_METADIRECTIVE:
> + {
> + for (unsigned i = 0; i < gimple_num_ops (stmt); i++)
> + {
> + label = gimple_omp_metadirective_label (stmt, i);
> + new_label = main_block_label (label, label_for_bb);
> + if (new_label != label)
> + gimple_omp_metadirective_set_label (stmt, i, new_label);
> + }
> + }
Why the {}s around this all? Just the for loop would do.
> + break;
> @@ -6329,6 +6341,18 @@ gimple_redirect_edge_and_branch (edge e, basic_block
> dest)
> gimple_block_label (dest));
> break;
>
> + case GIMPLE_OMP_METADIRECTIVE:
> + {
> + for (unsigned i = 0; i < gimple_num_ops (stmt); i++)
> + {
> + tree label = gimple_omp_metadirective_label (stmt, i);
> + if (label_to_block (cfun, label) == e->dest)
> + gimple_omp_metadirective_set_label (stmt, i,
> + gimple_block_label (dest));
> + }
> + }
Likewise.
> + break;
> + case GIMPLE_OMP_METADIRECTIVE:
> + /* The actual instruction will disappear eventually, so metadirective
> + statements have zero additional cost (if only static selectors
> + are used). */
> + /* TODO: Estimate the cost of evaluating dynamic selectors */
Missing . after selectors
> + return 0;
> +
Jakub
