Re: [PATCH, OpenMP, v2] Implement uses_allocators clause for target regions

[Jakub Jelinek <jakub@redhat.com>]

On Mon, May 30, 2022 at 10:43:30PM +0800, Chung-Lin Tang wrote:
> > This feels like you only accept a single allocator in the new syntax,
> > but that isn't my reading of the spec, I'd understand it as:
> > uses_allocators (memspace(omp_high_bw_mem_space), traits(foo_traits) : bar, baz, qux)
> > being valid too.
> 
> This patch now allows multiple allocators to be specified in new syntax, although I have
> to note that the 5.2 specification of uses_allocators (page 181) specifically says
> "allocator: expression of allocator_handle_type" for the "Arguments" description,
> not a "list" like the allocate clause.

I guess this should be raised on omp-lang then what we really want.
Because the 5.1 syntax definitely allowed multiple allocators.

> > It should be only removed if we emit the error (again with break; too).
> > IMHO (see the other mail) we should complain here if it has value 0
> > (the omp_null_allocator case), dunno if we should error or just warn
> > if the value is outside of the range of known predefined identifiers (1..8
> > currently I think). > But, otherwise, IMHO we need to keep it around, perhaps replacing the
> > CONST_DECL with INTEGER_CST, for the purposes of checking what predefined
> > allocators are used in the region.
> 
> omp_alloc in libgomp does handle the omp_null_allocator case, by converting it
> to something else.

Sure, but the spec says that omp_alloc (42, omp_null_allocator) is invalid
in target regions unless requires dynamic_allocators is seen first.
And uses_allocators (omp_null_allocator) shouldn't make that valid.
> @@ -15651,6 +15653,199 @@ c_parser_omp_clause_allocate (c_parser *parser, tree list)
>    return nl;
>  }
>  
> +/* OpenMP 5.0:
> +   uses_allocators ( allocator-list )
> +
> +   allocator-list:
> +   allocator
> +   allocator , allocator-list
> +   allocator ( traits-array )
> +   allocator ( traits-array ) , allocator-list
> +
> +   OpenMP 5.2:
> +
> +   uses_allocators ( modifier : allocator-list )
> +   uses_allocators ( modifier , modifier : allocator-list )
> +
> +   modifier:
> +   traits ( traits-array )
> +   memspace ( mem-space-handle )  */
> +
> +static tree
> +c_parser_omp_clause_uses_allocators (c_parser *parser, tree list)
> +{
> +  location_t clause_loc = c_parser_peek_token (parser)->location;
> +  tree t = NULL_TREE, nl = list;
> +  matching_parens parens;
> +  if (!parens.require_open (parser))
> +    return list;
> +
> +  tree memspace_expr = NULL_TREE;
> +  tree traits_var = NULL_TREE;
> +
> +  struct item_tok
> +  {
> +    location_t loc;
> +    tree id;
> +    item_tok (void) : loc (UNKNOWN_LOCATION), id (NULL_TREE) {}
> +  };
> +  struct item { item_tok name, arg; };
> +  auto_vec<item> *modifiers = NULL, *allocators = NULL;
> +  auto_vec<item> *cur_list = new auto_vec<item> (4);

Each vec/auto_vec with a new type brings quite some overhead,
a lot of functions need to be instantiated for it.
I think it would be far easier to use raw token parsing:
  unsigned int pos = 1;
  bool has_modifiers = false;
  while (true)
    {
      c_token *tok = c_parser_peek_nth_token_raw (parser, pos);
      if (tok->type != CPP_NAME)
	break;
      ++pos;
      if (c_parser_peek_nth_token_raw (parser, pos + 1)->type
	  == CPP_OPEN_PAREN)
	{
	  ++pos;
	  if (!c_parser_check_balanced_raw_token_sequence (parser, &pos)
	      || c_parser_peek_nth_token_raw (parser, pos)->type
		 != CPP_CLOSE_PAREN)
	    break;
	  ++pos;
	}
      tok = c_parser_peek_nth_token_raw (parser, pos);
      if (tok->type == CPP_COLON)
	{
	  has_modifiers = true;
	  break;
	}
      if (tok->type != CPP_COMMA)
	break;
      ++pos;
    }
This should (haven't tested it though, so sorry if there are errors)
cheaply determine if one should or shouldn't parse modifiers and
then can just do parsing of modifiers if (has_modifiers) and
then just the list (with ()s inside of list only if (!has_modifiers)).
> @@ -21093,7 +21292,8 @@ c_parser_omp_target_exit_data (location_t loc, c_parser *parser,
>  	| (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_IN_REDUCTION)	\
>  	| (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_THREAD_LIMIT)	\
>  	| (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_IS_DEVICE_PTR)\
> -	| (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_HAS_DEVICE_ADDR))
> +	| (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_HAS_DEVICE_ADDR)\

Can you please fix up both the IS_DEVICE_PTR and newly added
HAS_DEVICE_ADDR change to have a space before \ ?

> +	| (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_USES_ALLOCATORS))

> +static tree
> +cp_parser_omp_clause_uses_allocators (cp_parser *parser, tree list)
> +{
> +  location_t clause_loc
> +    = cp_lexer_peek_token (parser->lexer)->location;
> +  tree t = NULL_TREE, nl = list;
> +  matching_parens parens;
> +  if (!parens.require_open (parser))
> +    return list;
> +
> +  tree memspace_expr = NULL_TREE;
> +  tree traits_var = NULL_TREE;
> +
> +  struct item_tok
> +  {
> +    location_t loc;
> +    tree id;
> +    item_tok (void) : loc (UNKNOWN_LOCATION), id (NULL_TREE) {}
> +  };
> +  struct item { item_tok name, arg; };
> +  auto_vec<item> *modifiers = NULL, *allocators = NULL;
> +  auto_vec<item> *cur_list = new auto_vec<item> (4);

In C++ FE one can use tentative parsing instead, but nth_token
will work too.

  size_t pos = 1;
  bool has_modifiers = false;
  while (true)
    {
      if (!cp_lexer_nth_token_is (parser, pos, CPP_NAME))
	break;
      ++pos;
      if (cp_lexer_nth_token_is (parser, pos, CPP_OPEN_PAREN))
	{
	  size_t npos = cp_parser_skip_balanced_tokens (parser, pos);
	  if (npos == pos)
	    break;
	  pos = npos;
	}
      cp_token *tok = cp_lexer_peek_nth_token (parser, pos);
      if (tok->type == CPP_COLON)
	{
	  has_modifiers = true;
	  break;
	}
      if (tok->type != CPP_COMMA)
	break;
      ++pos;
    }

> @@ -44464,7 +44684,8 @@ cp_parser_omp_target_update (cp_parser *parser, cp_token *pragma_tok,
>  	| (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_IN_REDUCTION)	\
>  	| (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_THREAD_LIMIT)	\
>  	| (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_IS_DEVICE_PTR)\
> -	| (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_HAS_DEVICE_ADDR))
> +	| (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_HAS_DEVICE_ADDR)\

Ditto.

> +	case OMP_CLAUSE_USES_ALLOCATORS:
> +	  t = OMP_CLAUSE_USES_ALLOCATORS_ALLOCATOR (c);
> +	  if (TREE_CODE (t) == FIELD_DECL)
> +	    {
> +	      sorry_at (OMP_CLAUSE_LOCATION (c), "class members not yet "
> +			"supported in %<uses_allocators%> clause");
> +	      remove = true;
> +	      break;
> +	    }
> +	  t = convert_from_reference (t);

Are you sure about the above line?  Should we allow omp_allocator_handle_t &
type vars in the list?

> +	  if (TREE_CODE (TREE_TYPE (t)) != ENUMERAL_TYPE
> +	      || strcmp (IDENTIFIER_POINTER (TYPE_IDENTIFIER (TREE_TYPE (t))),
> +			 "omp_allocator_handle_t") != 0)

On the other side, if type_dependent_expression_p (t), I think we shouldn't
diagnose this but postpone it till instantiation.
And there should be in the testsuite a C++ testcase, where it
uses_allocators inside of a template, in one place with a non-dependent
omp_allocator_handle_t type, in another case e.g. with template parameter
type that is later instantiated with omp_allocator_handle_t type.

> +	    {
> +	      error_at (OMP_CLAUSE_LOCATION (c),
> +			"allocator must be of %<omp_allocator_handle_t%> type");
> +	      remove = true;
> +	      break;
> +	    }
> +	  if (TREE_CODE (t) == CONST_DECL)
> +	    {
> +	      /* Currently for pre-defined allocators in libgomp, we do not
> +		 require additional init/fini inside target regions, so discard
> +		 such clauses.  */
> +	      remove = true;

As I said earlier, I'd prefer to keep them and if for now you don't want to
warn for uses of allocators that aren't mentioned in uses_allocators, just
ignore them when actually privatizing them.

> +
> +	      if (OMP_CLAUSE_USES_ALLOCATORS_MEMSPACE (c)
> +		  || OMP_CLAUSE_USES_ALLOCATORS_TRAITS (c))
> +		{
> +		  error_at (OMP_CLAUSE_LOCATION (c),
> +			    "modifiers cannot be used with pre-defined "
> +			    "allocators");

But of course this case should have remove = true;

	Jakub