Hi Jakub,
this is v3 of the uses_allocators patch.

On 2022/5/20 1:46 AM, Jakub Jelinek wrote:
> 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:

Done.

>> +  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.

The parser parts have been rewritten to allow this kind of use now.
New code essentially parses lists of "id(id), id(id), ...", possibly delimited
by a ':' marking the modifier/allocator lists.

> 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.

Now everything is done in loops. The new code should be considerably simpler now.

> 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.

>> +	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.

Guarded now.

>> +	    {
>> +	      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;

Added some such breaks.

>> +	  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.

omp_alloc in libgomp does handle the omp_null_allocator case, by converting it
to something else.

The code already checks if the type is the OpenMP specified 'omp_memspace_handle_t'
enumeration type. A CONST_DECL of that type should be guaranteed a pre-defined
identifier.

>> +	  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[];

I've added a DECL_SIZE == NULL_TREE check, although the TREE_READONLY check for
the element type usually seems to already disqualify this case (because of the 'extern')

> 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.

Our interpretation of the requirement of "same-scope", and that it must be a constant array,
is that the traits array is intended to be inlined into the target region (instead of
more hassle issues related to transporting it to the offload target), which is what we
do right now.

In this case "same scope" is probably a little bit of overkill, it only needs to be staticly
known/computable by the compiler.

> 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.

Maybe later, I've sorry'ed FIELD_DECLs for now.

Asides from the review issues, this patch also includes some fixes for the allocate clause
firstprivate transfering on task constructs, triggered by the changes in the Fortran FE.

Tested without regressions on mainline.

Thanks,
Chung-Lin

2022-05-30  Chung-Lin Tang  <cltang@codesourcery.com>

gcc/c-family/ChangeLog:

	* c-omp.cc (c_omp_split_clauses): Add OMP_CLAUSE_USES_ALLOCATORS case.
	* c-pragma.h (enum pragma_omp_clause): Add PRAGMA_OMP_CLAUSE_USES_ALLOCATORS.

gcc/c/ChangeLog:

	* c-parser.cc (c_parser_omp_clause_name): Add case for uses_allocators
	clause.
	(c_parser_omp_clause_uses_allocators): New function.
	(c_parser_omp_all_clauses): Add PRAGMA_OMP_CLAUSE_USES_ALLOCATORS case.
	(OMP_TARGET_CLAUSE_MASK): Add PRAGMA_OMP_CLAUSE_USES_ALLOCATORS to mask.
	* c-typeck.cc (c_finish_omp_clauses): Add case handling for
	OMP_CLAUSE_USES_ALLOCATORS.

gcc/cp/ChangeLog:

	* parser.cc (cp_parser_omp_clause_name): Add case for uses_allocators
	clause.
	(cp_parser_omp_clause_uses_allocators): New function.
	(cp_parser_omp_all_clauses): Add PRAGMA_OMP_CLAUSE_USES_ALLOCATORS case.
	(OMP_TARGET_CLAUSE_MASK): Add PRAGMA_OMP_CLAUSE_USES_ALLOCATORS to mask.
	* semantics.cc (finish_omp_clauses): Add case handling for
	OMP_CLAUSE_USES_ALLOCATORS.

fortran/ChangeLog:

	* gfortran.h (struct gfc_omp_namelist): Add memspace_sym, traits_sym
	fields.
	(OMP_LIST_USES_ALLOCATORS): New list enum.
	* openmp.cc (enum omp_mask2): Add OMP_CLAUSE_USES_ALLOCATORS.
	(gfc_match_omp_clause_uses_allocators): New function.
	(gfc_match_omp_clauses): Add case to handle OMP_CLAUSE_USES_ALLOCATORS.
	(OMP_TARGET_CLAUSES): Add OMP_CLAUSE_USES_ALLOCATORS.
	(resolve_omp_clauses): Add "USES_ALLOCATORS" to clause_names[].
	* dump-parse-tree.cc (show_omp_namelist): Handle OMP_LIST_USES_ALLOCATORS.
	(show_omp_clauses): Likewise.
	* trans-array.cc (gfc_conv_array_initializer): Adjust array index
	to always be a created tree expression instead of NULL_TREE when zero.
	* trans-openmp.cc (gfc_trans_omp_clauses): For ALLOCATE clause, handle
	using gfc_trans_omp_variable for EXPR_VARIABLE exprs.
	Add handling of OMP_LIST_USES_ALLOCATORS case.
	* types.def (BT_FN_VOID_PTRMODE): Define.
	(BT_FN_PTRMODE_PTRMODE_INT_PTR): Define.

gcc/ChangeLog:

	* builtin-types.def (BT_FN_VOID_PTRMODE): Define.
	(BT_FN_PTRMODE_PTRMODE_INT_PTR): Define.
	* omp-builtins.def (BUILT_IN_OMP_INIT_ALLOCATOR): Define.
	(BUILT_IN_OMP_DESTROY_ALLOCATOR): Define.
	* tree-core.h (enum omp_clause_code): Add OMP_CLAUSE_USES_ALLOCATORS.
	* tree-pretty-print.cc (dump_omp_clause): Handle OMP_CLAUSE_USES_ALLOCATORS.
	* tree.h (OMP_CLAUSE_USES_ALLOCATORS_ALLOCATOR): New macro.
	(OMP_CLAUSE_USES_ALLOCATORS_MEMSPACE): New macro.
	(OMP_CLAUSE_USES_ALLOCATORS_TRAITS): New macro.
	* tree.cc (omp_clause_num_ops): Add OMP_CLAUSE_USES_ALLOCATORS.
	(omp_clause_code_name): Add "uses_allocators".

	* gimplify.cc (gimplify_scan_omp_clauses): Add checking of OpenMP target
	region allocate	clauses, to require a uses_allocators clause to exist
	for allocators.
	(gimplify_omp_workshare): Add handling of OMP_CLAUSE_USES_ALLOCATORS
	for OpenMP target regions; create calls of omp_init/destroy_allocator
	around target region body.
	* omp-low.cc (lower_private_allocate): Adjust receiving of allocator.
	(lower_rec_input_clauses): Likewise.
	(create_task_copyfn): Add dereference for allocator if needed.

gcc/testsuite/ChangeLog:

	* c-c++-common/gomp/uses_allocators-1.c: New test.
	* c-c++-common/gomp/uses_allocators-2.c: New test.
	* c-c++-common/gomp/uses_allocators-3.c: New test.
	* gfortran.dg/gomp/allocate-1.f90: Adjust testcase.
	* gfortran.dg/gomp/uses_allocators-1.f90: New test.
	* gfortran.dg/gomp/uses_allocators-2.f90: New test.
	* gfortran.dg/gomp/uses_allocators-3.f90: New test.