Re: [PATCH 2/7] openmp: Add middle-end support for metadirectives

[Jakub Jelinek <jakub@redhat.com>]

On Fri, Dec 10, 2021 at 05:33:25PM +0000, Kwok Cheung Yeung wrote:
> 2021-12-10  Kwok Cheung Yeung  <kcy@codesourcery.com>
> 
> 	gcc/
> 	* gimple-low.c (lower_omp_metadirective): New.
> 	(lower_stmt): Handle GIMPLE_OMP_METADIRECTIVE.
> 	* gimple-pretty-print.c (dump_gimple_omp_metadirective): New.
> 	(pp_gimple_stmt_1): Handle GIMPLE_OMP_METADIRECTIVE.
> 	* gimple-walk.c (walk_gimple_op): Handle GIMPLE_OMP_METADIRECTIVE.
> 	(walk_gimple_stmt): Likewise.
> 	* gimple.c (gimple_alloc_omp_metadirective): New.
> 	(gimple_build_omp_metadirective): New.
> 	(gimple_build_omp_metadirective_variant): New.
> 	* gimple.def (GIMPLE_OMP_METADIRECTIVE): New.
> 	(GIMPLE_OMP_METADIRECTIVE_VARIANT): New.
> 	* gimple.h (gomp_metadirective_variant): New.
> 	(gomp_metadirective): New.
> 	(is_a_helper <gomp_metadirective *>::test): New.
> 	(is_a_helper <gomp_metadirective_variant *>::test): New.
> 	(is_a_helper <const gomp_metadirective *>::test): New.
> 	(is_a_helper <const gomp_metadirective_variant *>::test): New.
> 	(gimple_alloc_omp_metadirective): New prototype.
> 	(gimple_build_omp_metadirective): New prototype.
> 	(gimple_build_omp_metadirective_variant): New prototype.
> 	(gimple_has_substatements): Add GIMPLE_OMP_METADIRECTIVE case.
> 	(gimple_has_ops): Add GIMPLE_OMP_METADIRECTIVE.
> 	(gimple_omp_metadirective_label): New.
> 	(gimple_omp_metadirective_set_label): New.
> 	(gimple_omp_metadirective_variants): New.
> 	(gimple_omp_metadirective_set_variants): New.
> 	(CASE_GIMPLE_OMP): Add GIMPLE_OMP_METADIRECTIVE.
> 	* gimplify.c (is_gimple_stmt): Add OMP_METADIRECTIVE.
> 	(expand_omp_metadirective): New.
> 	(gimplify_omp_metadirective): New.
> 	(gimplify_expr): Add case for OMP_METADIRECTIVE.
> 	* gsstruct.def (GSS_OMP_METADIRECTIVE): New.
> 	(GSS_OMP_METADIRECTIVE_VARIANT): New.
> 	* omp-expand.c (build_omp_regions_1): Handle GIMPLE_OMP_METADIRECTIVE.
> 	(omp_make_gimple_edges): Likewise.
> 	* omp-low.c (struct omp_context): Add next_clone field.
> 	(new_omp_context): Initialize next_clone field.
> 	(clone_omp_context): New.
> 	(delete_omp_context): Delete clone contexts.
> 	(scan_omp_metadirective): New.
> 	(scan_omp_1_stmt): Handle GIMPLE_OMP_METADIRECTIVE.
> 	(lower_omp_metadirective): New.
> 	(lower_omp_1): Handle GIMPLE_OMP_METADIRECTIVE.
> 	* tree-cfg.c (cleanup_dead_labels): Handle GIMPLE_OMP_METADIRECTIVE.
> 	(gimple_redirect_edge_and_branch): Likewise.
> 	* tree-inline.c (remap_gimple_stmt): Handle GIMPLE_OMP_METADIRECTIVE.
> 	(estimate_num_insns): Likewise.
> 	* tree-pretty-print.c (dump_generic_node): Handle OMP_METADIRECTIVE.
> 	* tree-ssa-operands.c (parse_ssa_operands): Handle
> 	GIMPLE_OMP_METADIRECTIVE.

> --- a/gcc/gimple-pretty-print.c
> +++ b/gcc/gimple-pretty-print.c
> @@ -2051,6 +2051,63 @@ dump_gimple_omp_return (pretty_printer *buffer, const gimple *gs, int spc,
>      }
>  }
>  
> +/* Dump a GIMPLE_OMP_METADIRECTIVE tuple on the pretty_printer BUFFER.  */
> +
> +static void
> +dump_gimple_omp_metadirective (pretty_printer *buffer, const gimple *gs,
> +			       int spc, dump_flags_t flags)
> +{
> +  if (flags & TDF_RAW)
> +    {
> +      dump_gimple_fmt (buffer, spc, flags, "%G <%+BODY <%S> >", gs,
> +		       gimple_omp_body (gs));
> +    }

No need for {}s around a single statement.

> +  else
> +    {
> +      pp_string (buffer, "#pragma omp metadirective");
> +      newline_and_indent (buffer, spc + 2);
> +
> +      gimple *variant = gimple_omp_metadirective_variants (gs);
> +
> +      for (unsigned i = 0; i < gimple_num_ops (gs); i++)
> +	{
> +	  tree selector = gimple_op (gs, i);
> +
> +	  if (selector == NULL_TREE)
> +	    pp_string (buffer, "default:");
> +	  else
> +	    {
> +	      pp_string (buffer, "when (");
> +	      dump_generic_node (buffer, selector, spc, flags, false);
> +	      pp_string (buffer, "):");
> +	    }
> +
> +	  if (variant != NULL)
> +	    {
> +	      newline_and_indent (buffer, spc + 4);
> +	      pp_left_brace (buffer);
> +	      pp_newline (buffer);
> +	      dump_gimple_seq (buffer, gimple_omp_body (variant), spc + 6,
> +			       flags);
> +	      newline_and_indent (buffer, spc + 4);
> +	      pp_right_brace (buffer);
> +
> +	      variant = variant->next;
> +	    }
> +	  else
> +	    {
> +	      tree label = gimple_omp_metadirective_label (gs, i);
> +
> +	      pp_string (buffer, " ");
> +	      dump_generic_node (buffer, label, spc, flags, false);
> +	    }
> +
> +	  if (i != gimple_num_ops (gs) - 1)
> +	    newline_and_indent (buffer, spc + 2);

I think better would be to use a gimple_stmt_iterator to walk the variants,
so no variant->next etc., but gimple_omp_metadirective_variants returning
a gimple_seq instead of gimple * (it is the same thing under the hood),
then
      gimple_seq variant_seq = gimple_omp_metadirective_variants (gs);
      gimple_stmt_iterator gsi = gsi_start (variant_seq);
and in the loop
	  gimple *variant = gsi_stmt (gsi);
and gsi_next (&gsi); at the end.  Similarly for all other spots that walk
gimple_omp_metadirective_variants.


> +    case GIMPLE_OMP_METADIRECTIVE:
> +      {
> +	gimple *variant = gimple_omp_metadirective_variants (stmt);
> +
> +	while (variant)
> +	  {
> +	    ret = walk_gimple_op (gimple_omp_body (variant), callback_op, wi);
> +	    if (ret)
> +	      return ret;
> +
> +	    variant = variant->next;
> +	  }

So here too...

> +      }
> +      break;
> +
>      case GIMPLE_TRANSACTION:
>        {
>  	gtransaction *txn = as_a <gtransaction *> (stmt);
> @@ -700,6 +715,22 @@ walk_gimple_stmt (gimple_stmt_iterator *gsi, walk_stmt_fn callback_stmt,
>  	return wi->callback_result;
>        break;
>  
> +    case GIMPLE_OMP_METADIRECTIVE:
> +      {
> +	gimple *variant = gimple_omp_metadirective_variants (stmt);
> +
> +	while (variant)
> +	  {
> +	    ret = walk_gimple_seq_mod (gimple_omp_body_ptr (variant),
> +				       callback_stmt, callback_op, wi);
> +	    if (ret)
> +	      return wi->callback_result;
> +
> +	    variant = variant->next;
> +	  }

and here etc.
> --- a/gcc/omp-expand.c
> +++ b/gcc/omp-expand.c
> @@ -10418,6 +10418,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.  */
> +	}

No {}s around the comment, just /* ...  */;

> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -1670,6 +1670,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);
> +	      }
> +	  }
> +	  break;

Why the {} around the for?

> @@ -6147,6 +6159,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));
> +	  }
> +      }
> +      break;

Likewise.

> --- a/gcc/tree-ssa-operands.c
> +++ b/gcc/tree-ssa-operands.c
> @@ -973,6 +973,33 @@ operands_scanner::parse_ssa_operands ()
>        append_vuse (gimple_vop (fn));
>        goto do_default;
>  
> +    case GIMPLE_OMP_METADIRECTIVE:
> +      n = gimple_num_ops (stmt);
> +      for (i = start; i < n; i++)
> +	{

Why the {}s around the inner for?

> +	  for (tree selector = gimple_op (stmt, i);
> +	       selector != NULL;
> +	       selector = TREE_CHAIN (selector))
> +	    {

Why the {}s around the if ?

> +	      if (TREE_PURPOSE (selector) == get_identifier ("user"))
> +		{
> +		  for (tree property = TREE_VALUE (selector);
> +		       property != NULL;
> +		       property = TREE_CHAIN (property))
> +		    if (TREE_PURPOSE (property)
> +			== get_identifier ("condition"))
> +		      {
> +			for (tree condition = TREE_VALUE (property);
> +			     condition != NULL;
> +			     condition = TREE_CHAIN (condition))
> +			  get_expr_operands (&TREE_VALUE (condition),
> +					     opf_use);
> +		      }
> +		}
> +	    }
> +	}
> +      break;
> +

Also, I wonder how does LTO saving/restoring handle the
GIMPLE_OMP_METADIRECTIVE statements.

Otherwise LGTM.

	Jakub