Re: [PATCH 4/7] openmp: Add support for streaming metadirectives and resolving them after LTO

[Jakub Jelinek <jakub@redhat.com>]

On Fri, Dec 10, 2021 at 05:36:20PM +0000, Kwok Cheung Yeung wrote:
> 2021-12-10  Kwok Cheung Yeung  <kcy@codesourcery.com>
> 
> 	gcc/
> 	* Makefile.in (OBJS): Add omp-expand-metadirective.o.
> 	* gimple-streamer-in.c (input_gimple_stmt): Add case for
> 	GIMPLE_OMP_METADIRECTIVE.  Handle metadirective labels.
> 	* gimple-streamer-out.c (output_gimple_stmt): Likewise.
> 	* omp-expand-metadirective.cc: New.
> 	* passes.def: Add pass_omp_expand_metadirective.
> 	* tree-pass.h (make_pass_omp_expand_metadirective): New prototype.
> ---
>  gcc/Makefile.in                 |   1 +
>  gcc/gimple-streamer-in.c        |  10 ++
>  gcc/gimple-streamer-out.c       |   6 +
>  gcc/omp-expand-metadirective.cc | 191 ++++++++++++++++++++++++++++++++
>  gcc/passes.def                  |   1 +
>  gcc/tree-pass.h                 |   1 +
>  6 files changed, 210 insertions(+)
>  create mode 100644 gcc/omp-expand-metadirective.cc
> 
> @@ -151,6 +151,7 @@ input_gimple_stmt (class lto_input_block *ib, class data_in *data_in,
>      case GIMPLE_COND:
>      case GIMPLE_GOTO:
>      case GIMPLE_DEBUG:
> +    case GIMPLE_OMP_METADIRECTIVE:
>        for (i = 0; i < num_ops; i++)
>  	{
>  	  tree *opp, op = stream_read_tree (ib, data_in);
> @@ -188,6 +189,15 @@ input_gimple_stmt (class lto_input_block *ib, class data_in *data_in,
>  	  else
>  	    gimple_call_set_fntype (call_stmt, stream_read_tree (ib, data_in));
>  	}
> +      if (gomp_metadirective *metadirective_stmt
> +	    = dyn_cast <gomp_metadirective*> (stmt))
> +	{
> +	  gimple_alloc_omp_metadirective (metadirective_stmt);
> +	  for (i = 0; i < num_ops; i++)
> +	    gimple_omp_metadirective_set_label (metadirective_stmt, i,
> +						stream_read_tree (ib,
> +								  data_in));
> +	}

Ah, sorry for the comment about LTO streaming, here it is.

> --- /dev/null
> +++ b/gcc/omp-expand-metadirective.cc
> @@ -0,0 +1,191 @@
> +/* Expand an OpenMP metadirective.
> +
> +   Copyright (C) 2021 Free Software Foundation, Inc.

We have 2022 now...
> +

Missing function comment.
> +static void
> +omp_expand_metadirective (function *fun, basic_block bb)
> +{
> +  gimple *stmt = last_stmt (bb);
> +  vec<struct omp_metadirective_variant> candidates
> +    = omp_resolve_metadirective (stmt);
> +
> +  /* This is the last chance for the metadirective to be resolved.  */
> +  if (candidates.is_empty ())
> +    gcc_unreachable ();

  gcc_assert (!candidates.is_empty ());
?

> +  /* opt_pass methods: */
> +  virtual bool gate (function *)
> +  {
> +    return (flag_openmp);

Useless ()s around it.
But much more importantly, I don't really like this to be a separate pass,
walking the whole IL once more is expensive, even when you restrict it
to just flag_openmp.
Late declare variant resolving is done in the (now a little bit misnamed)
pass_omp_device_lower.
The gate of that pass is right now:
      return (!(fun->curr_properties & PROP_gimple_lomp_dev)
              || (flag_openmp
                  && cgraph_node::get (fun->decl)->calls_declare_variant_alt));
so it would be nice to track (conservatively)
whether current function has any metadirectives
in it which aren't yet resolved (but perhaps the calls_declare_variant_alt
bit could be abused for that too) andin that case also deal with those.
You can surely gather them in the omp-offload.cc pass and then call
a function in your new file to handle that.

	Jakub