Re: [PATCH v2 09/11] OpenMP 5.0 "declare mapper" support for C++

[Jakub Jelinek <jakub@redhat.com>]

On Fri, Mar 18, 2022 at 09:26:50AM -0700, Julian Brown wrote:
> This patch implements OpenMP 5.0 "declare mapper" support for C++ --
> except for arrays of structs with mappers, which are TBD. I've taken cues
> from the existing "declare reduction" support where appropriate, though
> obviously the details of implementation differ somewhat (in particular,
> "declare mappers" must survive longer, until gimplification time).
> 
> Both named and unnamed (default) mappers are supported, and both
> explicitly-mapped structures and implicitly-mapped struct-typed variables
> used within an offload region are supported. For the latter, we need a
> way to communicate to the middle end which mapper (visible, in-scope) is
> the right one to use -- for that, we scan the target body in the front
> end for uses of structure (etc.) types, and create artificial "mapper
> binding" clauses to associate types with visible mappers. (It doesn't
> matter too much if we create these mapper bindings a bit over-eagerly,
> since they will only be used if needed later during gimplification.)
> 
> Another difficulty concerns the order in which an OpenMP offload region
> body's clauses are processed relative to its body: in order to add
> clauses for instantiated mappers, we need to have processed the body
> already in order to find which variables have been used, but at present
> the region's clauses are processed strictly before the body. So, this
> patch adds a second clause-processing step after gimplification of the
> body in order to add clauses resulting from instantiated mappers.
> 
> This version of the patch improves detection of explicitly-mapped struct
> accesses which inhibit implicitly-triggered user-defined mappers for a
> target region.

Will start with a general comment, from looking at the dumps it seems
handling the mappers in the FE right away for explicit mapping clauses
and attaching mapper binding clauses for types that are (or could
conservatively be, including from the recursive mappers themselves) be
used in the target body and letting gimplification find those var in detail
and use mapper binding clauses to actually expand it looks like the right
approach to me.  As I raised in an earlier patch, a big question is if we
should do map clause sorting on gimplify_scan_omp_clauses or
gimplify_adjust_omp_clauses or both...
The conservative discovery of what types we might need to create mapper
binding clauses for should be probably done only if
!processing_template_decl.

One question is though if DECL_OMP_DECLARE_MAPPER_P should be a magic
FUNCTION_DECL or a magic TREE_STATIC VAR_DECL or say CONST_DECLs.
The reason for the choice of FUNCTION_DECLs for UDRs is that they actually
contain code, but for UDMs we don't need any code, all we need is some
decl to which we can somehow attach list of clauses and a placeholder
decl used in them.  Perhaps a magic VAR_DECL or CONST_DECL would be
cheaper than a FUNCTION_DECL...

> @@ -32193,6 +32197,16 @@ cp_parser_late_parsing_for_member (cp_parser* parser, tree member_function)
>  	  finish_function (/*inline_p=*/true);
>  	  cp_check_omp_declare_reduction (member_function);
>  	}
> +      else if (DECL_OMP_DECLARE_MAPPER_P (member_function))
> +	{
> +	  parser->lexer->in_pragma = true;
> +	  cp_parser_omp_declare_mapper_maplist (member_function, parser);
> +	  finish_function (/*inline_p=*/true);
> +	  cp_check_omp_declare_mapper (member_function);
> +	  /* If this is a template class, this forces the body of the mapper
> +	     to be instantiated.  */
> +	  DECL_PRESERVE_P (member_function) = 1;

UDRs don't do this.  Why aren't the clauses instantiated when we actually
need such a template?

> @@ -39509,11 +39522,27 @@ cp_parser_omp_clause_map (cp_parser *parser, tree list)
>  
>        if (cp_lexer_peek_nth_token (parser->lexer, pos + 1)->type == CPP_COMMA)
>  	pos++;
> +      else if ((cp_lexer_peek_nth_token (parser->lexer, pos + 1)->type
> +		== CPP_OPEN_PAREN)
> +	       && ((cp_lexer_peek_nth_token (parser->lexer, pos + 2)->type
> +		    == CPP_NAME)
> +		   || ((cp_lexer_peek_nth_token (parser->lexer, pos + 2)->type
> +			== CPP_KEYWORD)
> +		       && (cp_lexer_peek_nth_token (parser->lexer,
> +						    pos + 2)->keyword
> +			   == RID_DEFAULT)))
> +	       && (cp_lexer_peek_nth_token (parser->lexer, pos + 3)->type
> +		   == CPP_CLOSE_PAREN)
> +	       && (cp_lexer_peek_nth_token (parser->lexer, pos + 4)->type
> +		   == CPP_COMMA))

In this loop we don't need to be exact, all we want is find out
if the mapper-mdifier candidates are followed by : or not, the
actual parsing is done only later.  So, can't we just use
for CPP_OPEN_PAREN cp_parser_skip_balanced_tokens to move over
all the modifier's arguments?

	Jakub