Re: [PATCH 1/7] openmp: Add C support for parsing metadirectives

[Jakub Jelinek <jakub@redhat.com>]

On Fri, Dec 10, 2021 at 05:31:08PM +0000, Kwok Cheung Yeung wrote:
> This patch adds support for parsing metadirectives in the C parser.
> 
> Metadirectives are represented by a OMP_METADIRECTIVE tree node. It has a
> single operand (accessed by OMP_METADIRECTIVE_CLAUSES) which contains a

I think naming this OMP_METADIRECTIVE_CLAUSES when the operand isn't
a chain of OMP_CLAUSE trees is misleading, I think better would be
OMP_METADIRECTIVE_VARIANTS.

> I have removed support for the 'omp begin metadirective'..'omp end
> metadirective' form of the directive that was originally in the WIP patch.
> According to the spec, the only variant directives that can be used in this
> form must have an 'end <directive>' form (apart from the 'nothing'
> directive), and in C/C++, the only directive that we support with an end
> form is 'declare target', which we currently forbid since it is declarative.

I guess that is fine initially, but eventually we should have support
for parsing of omp begin metadirective and omp end metadirective even
if we just always sorry then.

> --- a/gcc/c/c-parser.c
> +++ b/gcc/c/c-parser.c
> @@ -1390,6 +1390,17 @@ c_parser_skip_to_end_of_block_or_statement (c_parser *parser)
>  	  ++nesting_depth;
>  	  break;
>  
> +	case CPP_OPEN_PAREN:
> +	  /* Track parentheses in case the statement is a standalone 'for'
> +	     statement - we want to skip over the semicolons separating the
> +	     operands.  */
> +	  nesting_depth++;
> +	  break;
> +
> +	case CPP_CLOSE_PAREN:
> +	  nesting_depth--;
> +	  break;
> +
>  	case CPP_PRAGMA:
>  	  /* If we see a pragma, consume the whole thing at once.  We
>  	     have some safeguards against consuming pragmas willy-nilly.

I find this hunk very risky, it is used in many places and I'm not convinced
that is the behavior we want everywhere else.
I'd say the options are either to copy the function and add this only to the
copy and use that in metadirective handling, or add a default bool argument
and only do something about nesting_depth if the argument is non-default.
Furthermore, I don't think we want to just blindly decrement nesting_depth,
the CPP_CLOSE_BRACE takes care of never decrementing it below zero.
And, the function uses ++nesting_depth etc. instead of nesting_depth++
so some consistency would be nice.

> @@ -19187,6 +19200,7 @@ c_parser_omp_for_loop (location_t loc, c_parser *parser, enum tree_code code,
>    location_t for_loc;
>    bool tiling = false;
>    bool inscan = false;
> +
>    vec<tree, va_gc> *for_block = make_tree_vector ();
>  
>    for (cl = clauses; cl; cl = OMP_CLAUSE_CHAIN (cl))

Why?

> @@ -21606,10 +21621,16 @@ c_parser_omp_context_selector (c_parser *parser, tree set, tree parms)
>  		{
>  		  mark_exp_read (t);
>  		  t = c_fully_fold (t, false, NULL);
> -		  if (!INTEGRAL_TYPE_P (TREE_TYPE (t))
> -		      || !tree_fits_shwi_p (t))
> +		  if (!metadirective_p
> +		      && (!INTEGRAL_TYPE_P (TREE_TYPE (t))
> +			  || !tree_fits_shwi_p (t)))
>  		    error_at (token->location, "property must be "
> -			      "constant integer expression");
> +					       "constant integer expression");
> +		  else if (metadirective_p
> +			   && !INTEGRAL_TYPE_P (TREE_TYPE (t)))
> +		    /* Allow non-constant user expressions in metadirectives.  */
> +		    error_at (token->location, "property must be "
> +					       "integer expression");
>  		  else
>  		    properties = tree_cons (NULL_TREE, t, properties);
>  		}

I don't understand this change.  In OpenMP 5.0, condition selector had to be
constant.  In OpenMP 5.1, it can be non-constant and then it is a dynamic
selector.  But there is no restriction that it must be constant for
declare variant.
I think enabling this is orthogonal to the metadirective support, so either
the initial version shouldn't support dynamic selectors and a follow-up
patch should add support for them for both metadirectives and declare
variant, or the support should be added for both at the same time.

> @@ -22930,6 +22953,368 @@ c_parser_omp_error (c_parser *parser, enum pragma_context context)
>    return false;
>  }
>  
> +/* Helper function for c_parser_omp_metadirective.  */
> +
> +static void
> +analyze_metadirective_body (c_parser *parser,
> +			    vec<c_token> &tokens,
> +			    vec<tree> &labels)
> +{
> +  int nesting_depth = 0;
> +  int bracket_depth = 0;
> +  bool ignore_label = false;
> +
> +  /* Read in the body tokens to the tokens for each candidate directive.  */
> +  while (1)
> +    {
> +      c_token *token = c_parser_peek_token (parser);
> +      bool stop = false;
> +
> +      if (c_parser_next_token_is_keyword (parser, RID_CASE))
> +	ignore_label = true;
> +
> +      switch (token->type)
> +	{
> +	case CPP_EOF:
> +	  break;
> +	case CPP_NAME:
> +	  if (!ignore_label
> +	      && c_parser_peek_2nd_token (parser)->type == CPP_COLON)
> +	    labels.safe_push (token->value);

This looks risky, not all CPP_NAME CPP_COLON adjacent tokens will be
actually labels.
E.g. in
  { struct S { int a, b; } c = { a: 1, b: 2 }; }
a and b aren't labels.
I'm afraid we need real parsing to find out what is a label and what is not.
Or for C
  { void bar (void) { goto a; a:; } bar (); }
a is indeed a label, but in a nested function (for C++ e.g. in lambda)
and I doubt we want to privatize those either.
Gathering this way just potential label candidates and later marking only
those that were actually encountered during parsing might work.

> +c_parser_omp_metadirective (location_t loc, c_parser *parser,
> +			    char *p_name, omp_clause_mask, tree *,
> +			    bool *if_p)
> +{
> +  tree ret;
> +  auto_vec<c_token> directive_tokens;
> +  auto_vec<c_token> body_tokens;
> +  auto_vec<tree> body_labels;
> +  auto_vec<const struct c_omp_directive *> directives;
> +  auto_vec<tree> ctxs;
> +  vec<struct omp_metadirective_variant> candidates;
> +  bool default_seen = false;
> +  int directive_token_idx = 0;
> +  tree standalone_body = NULL_TREE;
> +
> +  ret = make_node (OMP_METADIRECTIVE);
> +  SET_EXPR_LOCATION (ret, loc);
> +  TREE_TYPE (ret) = void_type_node;
> +  OMP_METADIRECTIVE_CLAUSES (ret) = NULL_TREE;
> +  strcat (p_name, " metadirective");
> +
> +  while (c_parser_next_token_is_not (parser, CPP_PRAGMA_EOL))
> +    {
> +      if (c_parser_next_token_is_not (parser, CPP_NAME)
> +	  && c_parser_next_token_is_not (parser, CPP_KEYWORD))
> +	{
> +	  c_parser_error (parser, "expected %<when%> or %<default%>");

Consistency would suggest
"expected %<#pragma omp%> clause"
or, if you want to spell those out,
"expected %<when%> or %<default%> clause"
But note that we'll need to add %<otherwise%> clause to the list soon
as an alias to %<default%>.

> +	  goto error;
> +	}
> +
> +      location_t match_loc = c_parser_peek_token (parser)->location;
> +      const char *p = IDENTIFIER_POINTER (c_parser_peek_token (parser)->value);
> +      c_parser_consume_token (parser);
> +      bool default_p = strcmp (p, "default") == 0;
> +      if (default_p)
> +	{
> +	  if (default_seen)
> +	    {
> +	      c_parser_error (parser, "there can only be one default clause "
> +				      "in a metadirective");

As I said elsewhere, "too many %qs clauses", "default"

> +	      goto error;
> +	    }
> +	  default_seen = true;
> +	}
> +      if (!(strcmp (p, "when") == 0 || default_p))
> +	{
> +	  c_parser_error (parser, "expected %<when%> or %<default%>");

Consistency would suggest
error_at (here, "%qs is not valid for %qs", p, "metadirective");

> +	  /* Remove the selector from further consideration if can be
> +	     evaluated as a non-match at this point.  */
> +	  skip = (omp_context_selector_matches (ctx, true) == 0);

The outer ()s aren't needed.

> +
> +	  if (c_parser_next_token_is_not (parser, CPP_COLON))
> +	    {
> +	      c_parser_error (parser, "expected colon");
> +	      goto error;
> +	    }

c_parser_require (parser, CPP_COLON, "expected %<:%>")
instead?  Or at least "expected %<:%>"

> +      /* Read in the directive type and create a dummy pragma token for
> +	 it.  */
> +      location_t loc = c_parser_peek_token (parser)->location;
> +
> +      p = NULL;
> +      if (c_parser_next_token_is (parser, CPP_CLOSE_PAREN))
> +	p = "nothing";
> +      else if (c_parser_next_token_is_keyword (parser, RID_FOR))
> +	{
> +	  p = "for";
> +	  c_parser_consume_token (parser);
> +	}
> +      else if (c_parser_next_token_is (parser, CPP_NAME))
> +	{
> +	  p = IDENTIFIER_POINTER (c_parser_peek_token (parser)->value);
> +	  c_parser_consume_token (parser);
> +	}
> +
> +      if (p == NULL)
> +	{
> +	  c_parser_error (parser, "expected directive name");
> +	  goto error;
> +	}
> +
> +      const struct c_omp_directive *omp_directive
> +	= c_omp_categorize_directive (p, NULL, NULL);

The NULL, NULL looks wrong.  I don't see how you'd handle
say when (whatever: target enter data) correctly then,
or any other multiple identifier directive.
You should simply peek (raw) at the next token after it and
if it is also a CPP_NAME, supply the other name, and in that
case look for the token even after it and if it is CPP_NAME, supply
that too.
            const char *directive[3] = {};
            for (int i = 0; i < 3; i++)
              {
                tree id = NULL_TREE;
                if (first + i == last)
                  break;
                if (first[i].type == CPP_NAME)
                  id = first[i].u.value;
                else if (first[i].type == CPP_KEYWORD)
                  id = ridpointers[(int) first[i].keyword];
                else
                  break;
                directive[i] = IDENTIFIER_POINTER (id);
              }
is what the C++ FE does.
> +
> +      if (omp_directive == NULL)
> +	{
> +	  c_parser_error (parser, "unknown directive name");
> +	  goto error;
> +	}
> +      if (omp_directive->id == PRAGMA_OMP_METADIRECTIVE)
> +	{
> +	  c_parser_error (parser,
> +			  "metadirectives cannot be used as directive "
> +			  "variants");

"variants of a %<metadirective%>" ?

> +	  goto error;
> +	}
> +      if (omp_directive->kind == C_OMP_DIR_DECLARATIVE)
> +	{
> +	  sorry_at (loc, "declarative directive variants are not supported");

"declarative directive variants of a %<metadirective%> not supported" ?

> +  analyze_metadirective_body (parser, body_tokens, body_labels);

I think the code above this should determine if all the directives are
standalone/informational/utility and in that case don't try to analyze
any body, no?

> @@ -23043,7 +23433,6 @@ c_parser_omp_construct (c_parser *parser, bool *if_p)
>      gcc_assert (EXPR_LOCATION (stmt) != UNKNOWN_LOCATION);
>  }
>  
> -
>  /* OpenMP 2.5:
>     # pragma omp threadprivate (variable-list) */
>  

Why?

	Jakub