[PATCH, 1/3, OpenMP] Target mapping changes for OpenMP 5.0, front-end parts

[Jakub Jelinek]

On Tue, Sep 01, 2020 at 09:16:23PM +0800, Chung-Lin Tang wrote:
> this patch set implements parts of the target mapping changes introduced
> in OpenMP 5.0, mainly the attachment requirements for pointer-based
> list items, and the clause ordering.
> 
> The first patch here are the C/C++ front-end changes.
> 
> The entire set of changes has been tested for without regressions for
> the compiler and libgomp. Hope this is ready to commit to master.

Sorry for the delay in patch review and thanks for the standard citations,
that really helps.

>         gcc/c-family/
>         * c-common.h (c_omp_adjust_clauses): New declaration.
>         * c-omp.c (c_omp_adjust_clauses): New function.

Besides the naming, I wonder why is it done in a separate function and so
early, can't what the function does be done either in
{,c_}finish_omp_clauses (provided we'd pass separate ORT_OMP vs.
ORT_OMP_TARGET to it to determine if it is target region vs. anything else),
or perhaps even better during gimplification (gimplify_scan_omp_clauses)?

>         gcc/c/
>         * c-parser.c (c_parser_omp_target_data): Add use of
>         new c_omp_adjust_clauses function. Add GOMP_MAP_ATTACH_DETACH as
> 	handled map clause kind.
>         (c_parser_omp_target_enter_data): Likewise.
>         (c_parser_omp_target_exit_data): Likewise.
>         (c_parser_omp_target): Likewise.
>         * c-typeck.c (handle_omp_array_sections): Adjust COMPONENT_REF case to
>         use GOMP_MAP_ATTACH_DETACH map kind for C_ORT_OMP region type.
>         (c_finish_omp_clauses): Adjust bitmap checks to allow struct decl and
>         same struct field access to co-exist on OpenMP construct.
> 
>         gcc/cp/
>         * parser.c (cp_parser_omp_target_data): Add use of
>         new c_omp_adjust_clauses function. Add GOMP_MAP_ATTACH_DETACH as
>         handled map clause kind.
>         (cp_parser_omp_target_enter_data): Likewise.
> 	(cp_parser_omp_target_exit_data): Likewise.
> 	(cp_parser_omp_target): Likewise.
> 	* semantics.c (handle_omp_array_sections): Adjust COMPONENT_REF case to
> 	use GOMP_MAP_ATTACH_DETACH map kind for C_ORT_OMP region type. Fix
> 	interaction between reference case and attach/detach.
> 	(finish_omp_clauses): Adjust bitmap checks to allow struct decl and
> 	same struct field access to co-exist on OpenMP construct.

The changelog has some 8 space indented lines.

> +  for (tree c = clauses; c; c = OMP_CLAUSE_CHAIN (c))
> +    if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
> +	&& OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_FIRSTPRIVATE_POINTER
> +	&& TREE_CODE (TREE_TYPE (OMP_CLAUSE_DECL (c))) != ARRAY_TYPE)
> +      {
> +	tree ptr = OMP_CLAUSE_DECL (c);
> +	bool ptr_mapped = false;
> +	if (is_target)
> +	  {
> +	    for (tree m = clauses; m; m = OMP_CLAUSE_CHAIN (m))

Isn't this O(n^2) in number of clauses?  I mean, e.g. for the equality
comparisons (but see below) it could be dealt with e.g. using some bitmap
with DECL_UIDs.

> +	      if (OMP_CLAUSE_CODE (m) == OMP_CLAUSE_MAP
> +		  && OMP_CLAUSE_DECL (m) == ptr

Does it really need to be equality?  I mean it will be for
map(tofrom:ptr) map(tofrom:ptr[:32])
but what about e.g.
map(tofrom:structx) map(tofrom:structx.ptr[:32])
?  It is true that likely we don't parse this yet though.

> +		  && (OMP_CLAUSE_MAP_KIND (m) == GOMP_MAP_ALLOC
> +		      || OMP_CLAUSE_MAP_KIND (m) == GOMP_MAP_TO
> +		      || OMP_CLAUSE_MAP_KIND (m) == GOMP_MAP_FROM
> +		      || OMP_CLAUSE_MAP_KIND (m) == GOMP_MAP_TOFROM))

What about the always modified mapping kinds?

> +		{
> +		  ptr_mapped = true;
> +		  break;
> +		}
> +
> +	    if (!ptr_mapped
> +		&& DECL_P (ptr)
> +		&& is_global_var (ptr)
> +		&& lookup_attribute ("omp declare target",
> +				     DECL_ATTRIBUTES (ptr)))
> +	      ptr_mapped = true;
> +	  }
> +
> +	/* If the pointer variable was mapped, or if this is not an offloaded
> +	   target region, adjust the map kind to attach/detach.  */
> +	if (ptr_mapped || !is_target)
> +	  {
> +	    OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_ATTACH_DETACH);
> +	    c_common_mark_addressable_vec (ptr);

Though perhaps this is argument why it needs to be done in the FEs and not
during gimplification, because it is hard to mark something addressable at
that point.

> --- a/gcc/c/c-typeck.c
> +++ b/gcc/c/c-typeck.c
> @@ -13580,16 +13580,17 @@ handle_omp_array_sections (tree c, enum c_omp_region_type ort)
>  	    break;
>  	  }
>        tree c2 = build_omp_clause (OMP_CLAUSE_LOCATION (c), OMP_CLAUSE_MAP);
>        if (ort != C_ORT_OMP && ort != C_ORT_ACC)
>  	OMP_CLAUSE_SET_MAP_KIND (c2, GOMP_MAP_POINTER);
>        else if (TREE_CODE (t) == COMPONENT_REF)
>  	{
> -	  gomp_map_kind k = (ort == C_ORT_ACC) ? GOMP_MAP_ATTACH_DETACH
> -					       : GOMP_MAP_ALWAYS_POINTER;
> +	  gomp_map_kind k
> +	    = ((ort == C_ORT_ACC || ort == C_ORT_OMP)
> +	       ? GOMP_MAP_ATTACH_DETACH : GOMP_MAP_ALWAYS_POINTER);

So what kind of C_ORT_* would be left after this change? 
C_ORT_*DECLARE_SIMD shouldn't have any kind of array sections in it.
So maybe just

>  	  OMP_CLAUSE_SET_MAP_KIND (c2, k);
OMP_CLAUSE_SET_MAP_KIND (c2, GOMP_MAP_ALWAYS_POINTER);
?

>  	      if (VAR_P (t) || TREE_CODE (t) == PARM_DECL)
>  		{
> -		  if (bitmap_bit_p (&map_field_head, DECL_UID (t)))
> +		  if (bitmap_bit_p (&map_field_head, DECL_UID (t))
> +		      || bitmap_bit_p (&map_head, DECL_UID (t)))
>  		    break;

Shall this change apply to OpenACC too?

>  		}
>  	    }
>  	  if (!VAR_P (t) && TREE_CODE (t) != PARM_DECL)
>  	    {
>  	      error_at (OMP_CLAUSE_LOCATION (c),
>  			"%qE is not a variable in %qs clause", t,
> @@ -14751,29 +14753,36 @@ c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
>  		    error_at (OMP_CLAUSE_LOCATION (c),
>  			      "%qD appears both in data and map clauses", t);
>  		  remove = true;
>  		}
>  	      else
>  		bitmap_set_bit (&generic_head, DECL_UID (t));
>  	    }
> -	  else if (bitmap_bit_p (&map_head, DECL_UID (t)))
> +	  else if (bitmap_bit_p (&map_head, DECL_UID (t))
> +		   && !bitmap_bit_p (&map_field_head, DECL_UID (t)))

Ditto.  Otherwise, what shall this diagnose now that the restriction that
the same list item may not appear in multiple clauses is gone.

>  	    {
>  	      if (OMP_CLAUSE_CODE (c) != OMP_CLAUSE_MAP)
>  		error_at (OMP_CLAUSE_LOCATION (c),
>  			  "%qD appears more than once in motion clauses", t);
>  	      else if (ort == C_ORT_ACC)
>  		error_at (OMP_CLAUSE_LOCATION (c),
>  			  "%qD appears more than once in data clauses", t);
>  	      else
>  		error_at (OMP_CLAUSE_LOCATION (c),
>  			  "%qD appears more than once in map clauses", t);
>  	      remove = true;

So what is this supposed to diagnose now that the restriction that

C++ ditto.

	Jakub