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

[Jakub Jelinek <jakub@redhat.com>]

On Tue, May 10, 2022 at 07:29:23PM +0800, Chung-Lin Tang wrote:
> @@ -15624,6 +15626,233 @@ c_parser_omp_clause_allocate (c_parser *parser, tree list)
>    return nl;
>  }
>  
> +/* OpenMP 5.2:
> +   uses_allocators ( allocator-list )

As uses_allocators is a 5.0 feature already, the above should say
/* OpenMP 5.0:
> +
> +   allocator-list:
> +   allocator
> +   allocator , allocator-list
> +   allocator ( traits-array )
> +   allocator ( traits-array ) , allocator-list
> +

And here it should add
  OpenMP 5.2:

> +   uses_allocators ( modifier : allocator )
> +   uses_allocators ( modifier , modifier : allocator )
> +
> +   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;
> +
> +  bool has_modifiers = false;
> +  tree memspace_expr = NULL_TREE;
> +  tree traits_var = NULL_TREE;
> +
> +  if (c_parser_next_token_is (parser, CPP_NAME))
> +    {
> +      c_token *tok = c_parser_peek_token (parser);
> +      const char *p = IDENTIFIER_POINTER (tok->value);
> +
> +      if (strcmp ("traits", p) == 0 || strcmp ("memspace", p) == 0)
> +	{
> +	  has_modifiers = true;
> +	  c_parser_consume_token (parser);
> +	  matching_parens parens2;;

Double ;;, should be just ;
But more importantly, it is more complex.
When you see
uses_allocators(traits or
uses_allocators(memspace
it is not given that it has modifiers.  While the 5.0/5.1 syntax had
a restriction that when allocator is not a predefined allocator (and
traits or memspace aren't predefined allocators) it must use ()s with
traits, so
uses_allocators(traits)
uses_allocators(memspace)
uses_allocators(traits,memspace)
are all invalid,
omp_allocator_handle_t traits;
uses_allocators(traits(mytraits))
or
omp_allocator_handle_t memspace;
uses_allocators(memspace(mytraits),omp_default_mem_alloc)
are valid in the old syntax.

So, I'm afraid to find out if the traits or memspace identifier
seen after uses_allocator ( are modifiers or not we need to
peek (for C with c_parser_peek_nth_token_raw) through all the
modifiers whether we see a : and only in that case say they
are modifiers rather than the old style syntax.

> +	  parens2.require_open (parser);
> +
> +	  if (c_parser_next_token_is (parser, CPP_NAME)
> +	      && (c_parser_peek_token (parser)->id_kind == C_ID_ID
> +		  || c_parser_peek_token (parser)->id_kind == C_ID_TYPENAME))
> +	    {
> +	      tok = c_parser_peek_token (parser);
> +	      t = lookup_name (tok->value);
> +
> +	      if (t == NULL_TREE)
> +		{
> +		  undeclared_variable (tok->location, tok->value);
> +		  t = error_mark_node;
> +		}
> +	      else
> +		{
> +		  if (strcmp ("memspace", p) == 0)

I think it would be better to have bool variable whether
it was memspace or traits modifier, so strcmp just once
with each string, not multiple times.

In the 5.2 syntax, for memspace it is clear that it has to be
an identifier that is the predefined namespace, but for
traits it just says it is a variable of alloctrait array type,
it is unclear if it must be an identifier or could be say structure
element or array element etc.  Guess something to discuss on omp-lang
and for now pretend it must be an identifier.

> +	  if (c_parser_next_token_is (parser, CPP_COMMA))
> +	    {
> +	      c_parser_consume_token (parser);
> +	      tok = c_parser_peek_token (parser);
> +	      const char *q = "";
> +	      if (c_parser_next_token_is (parser, CPP_NAME))
> +		q = IDENTIFIER_POINTER (tok->value);
> +	      if (strcmp (q, "memspace") != 0 && strcmp (q, "traits") != 0)
> +		{
> +		  c_parser_error (parser, "expected %<memspace%> or %<traits%>");
> +		  parens.skip_until_found_close (parser);
> +		  return list;
> +		}

I don't really like the modifiers handling not done in a loop.
As I said above, there needs to be some check whether there are modifiers or
not, but once we figure out there are modifiers, it should be done in a loop
with say some mask var on which traits have been already handled to diagnose
duplicates, we don't want to do the parsing code twice.

> +  if (has_modifiers)
> +    {
> +      if (!c_parser_require (parser, CPP_COLON, "expected %<:%>"))
> +	{
> +	  parens.skip_until_found_close (parser);
> +	  return list;
> +	}
> +
> +      if (c_parser_next_token_is (parser, CPP_NAME)
> +	  && c_parser_peek_token (parser)->id_kind == C_ID_ID)
> +	{
> +	  tree t = lookup_name (c_parser_peek_token (parser)->value);
> +
> +	  if (t == NULL_TREE)
> +	    {
> +	      undeclared_variable (c_parser_peek_token (parser)->location,
> +				   c_parser_peek_token (parser)->value);
> +	      t = error_mark_node;
> +	    }
> +	  else if (t != error_mark_node)
> +	    {
> +	      tree c = build_omp_clause (clause_loc,
> +					 OMP_CLAUSE_USES_ALLOCATORS);
> +	      OMP_CLAUSE_USES_ALLOCATORS_ALLOCATOR (c) = t;
> +	      OMP_CLAUSE_USES_ALLOCATORS_MEMSPACE (c) = memspace_expr;
> +	      OMP_CLAUSE_USES_ALLOCATORS_TRAITS (c) = traits_var;
> +	      OMP_CLAUSE_CHAIN (c) = list;
> +
> +	      nl = c;
> +	    }
> +	  c_parser_consume_token (parser);
> +
> +	  if (c_parser_next_token_is (parser, CPP_OPEN_PAREN))
> +	    c_parser_error (parser, "modifiers cannot be used with "
> +			    "legacy array syntax");
> +	  else if (c_parser_next_token_is (parser, CPP_COMMA))
> +	    c_parser_error (parser, "modifiers can only be used with "
> +			    "a single allocator in %<uses_allocators%> "
> +			    "clause");
> +	}
> +      else
> +	c_parser_error (parser, "expected identifier");

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.

And, I'd strongly prefer to just c_parser_omp_variable_list at this point
for the rest if there were modifiers and just fill in
OMP_CLAUSE_USES_ALLOCATORS_MEMSPACE and OMP_CLAUSE_USES_ALLOCATORS_TRAITS
on each similarly to how e.g. linear or other clause with modifiers are
handled.

The no modifiers case of course needs its own code so that it handles the
()s.

> +	case OMP_CLAUSE_USES_ALLOCATORS:
> +	  t = OMP_CLAUSE_USES_ALLOCATORS_ALLOCATOR (c);
> +	  if (bitmap_bit_p (&generic_head, DECL_UID (t))
> +	      || bitmap_bit_p (&map_head, DECL_UID (t))
> +	      || bitmap_bit_p (&firstprivate_head, DECL_UID (t))
> +	      || bitmap_bit_p (&lastprivate_head, DECL_UID (t)))

You can't just use DECL_UID before you actually verify it is a variable.
So IMHO this particular if should be moved down somewhat.

> +	    {
> +	      error_at (OMP_CLAUSE_LOCATION (c),
> +			"%qE appears more than once in data clauses", t);
> +	      remove = true;
> +	    }
> +	  else
> +	    bitmap_set_bit (&generic_head, DECL_UID (t));
> +	  if (TREE_CODE (TREE_TYPE (t)) != ENUMERAL_TYPE
> +	      || strcmp (IDENTIFIER_POINTER (TYPE_IDENTIFIER (TREE_TYPE (t))),
> +			 "omp_allocator_handle_t") != 0)
> +	    {
> +	      error_at (OMP_CLAUSE_LOCATION (c),
> +			"allocator must be of %<omp_allocator_handle_t%> type");
> +	      remove = true;
> +	    }

I'd add break; after remove = true;

> +	  if (TREE_CODE (t) == CONST_DECL)
> +	    {
> +	      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");
> +
> +	      /* Currently for pre-defined allocators in libgomp, we do not
> +		 require additional init/fini inside target regions, so discard
> +		 such clauses.  */
> +	      remove = true;
> +	    }

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.

But break; afterwards to avoid the code below.

Then, do a VAR_DECL/PARM_DECL check, complain if it is not (see other spots
in the function that do check that).

Then the bitmap_bit_p stuff above.

> +	  t = OMP_CLAUSE_USES_ALLOCATORS_MEMSPACE (c);
> +	  if (t != NULL_TREE
> +	      && (TREE_CODE (t) != CONST_DECL
> +		  || TREE_CODE (TREE_TYPE (t)) != ENUMERAL_TYPE
> +		  || strcmp (IDENTIFIER_POINTER (TYPE_IDENTIFIER (TREE_TYPE (t))),
> +			     "omp_memspace_handle_t") != 0))
> +	    {
> +	      error_at (OMP_CLAUSE_LOCATION (c), "memspace modifier must be "
> +			"constant enum of %<omp_memspace_handle_t%> type");
> +	      remove = true;
> +	    }

Again, wonder if it shouldn't after checking it replace the CONST_DECL with
an INTEGER_CST for the purposes of the middle-end.

> +	  t = OMP_CLAUSE_USES_ALLOCATORS_TRAITS (c);
> +	  if (t != NULL_TREE)
> +	    {
> +	      bool type_err = false;
> +
> +	      if (TREE_CODE (TREE_TYPE (t)) != ARRAY_TYPE)
> +		type_err = true;
> +	      else
> +		{
> +		  tree elem_t = TREE_TYPE (TREE_TYPE (t));
> +		  if (TREE_CODE (elem_t) != RECORD_TYPE
> +		      || strcmp (IDENTIFIER_POINTER (TYPE_IDENTIFIER (elem_t)),
> +				 "omp_alloctrait_t") != 0
> +		      || !TYPE_READONLY (elem_t))

I'd diagnose if the array is incomplete, say
extern omp_alloctrait_t traits[];

For the 5.2 syntax, there is also the restriction that
"be defined in the same scope as the construct on which the clause appears"
which I don't see being checked here.  Unclear whether it applies to the
old syntax too.

But again, it should also check that it is a VAR_DECL, it isn't extern
etc.

For C++, I have to wonder if at allocator couldn't be a non-static data
member of a class inside of methods, that is something that can be generally
privatized too.

	Jakub