Re: [PATCH, OpenMP 5.0] Implement relaxation of implicit map vs. existing device mappings (for mainline trunk)

[Jakub Jelinek <jakub@redhat.com>]

On Fri, May 14, 2021 at 09:20:25PM +0800, Chung-Lin Tang wrote:
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index e790f08b23f..69c4a8e0a0a 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -10374,6 +10374,7 @@ gimplify_adjust_omp_clauses_1 (splay_tree_node n, void *data)
>  	  gcc_unreachable ();
>  	}
>        OMP_CLAUSE_SET_MAP_KIND (clause, kind);
> +      OMP_CLAUSE_MAP_IMPLICIT_P (clause) = 1;
>        if (DECL_SIZE (decl)
>  	  && TREE_CODE (DECL_SIZE (decl)) != INTEGER_CST)
>  	{

As Thomas mentioned, there is now also OMP_CLAUSE_MAP_IMPLICIT that means
something different:
/* Nonzero on map clauses added implicitly for reduction clauses on combined
   or composite constructs.  They shall be removed if there is an explicit
   map clause.  */
Having OMP_CLAUSE_MAP_IMPLICIT and OMP_CLAUSE_MAP_IMPLICIT_P would be too
confusing.  So either we need to use just one flag for both purposes or
have two different flags and find a better name for one of them.
The former would be possible if no OMP_CLAUSE_MAP clauses added by the FEs
are implicit - then you could clear OMP_CLAUSE_MAP_IMPLICIT in
gimplify_scan_omp_clauses.  I wonder if it is the case though, e.g. doesn't
your "Improve OpenMP target support for C++ [PR92120 v4]" patch add a lot of
such implicit map clauses (e.g. the this[:1] and various others)?
Also, gimplify_adjust_omp_clauses_1 sometimes doesn't add just one map
clause, but several, shouldn't those be marked implicit too?  And similarly
it calls lang_hooks.decls.omp_finish_clause which can add even further map
clauses implicitly, shouldn't those be implicit too (in that case copy
the flag from the clause it is called on to the extra clauses it adds)?

Also as Thomas mentioned, it should be restricted to non-OpenACC,
it can check gimplify_omp_ctxp->region_type if it is OpenMP or OpenACC.

> @@ -10971,9 +10972,15 @@ gimplify_adjust_omp_clauses (gimple_seq *pre_p, gimple_seq body, tree *list_p,
>  	list_p = &OMP_CLAUSE_CHAIN (c);
>      }
>  
> -  /* Add in any implicit data sharing.  */
> +  /* Add in any implicit data sharing. Implicit clauses are added at the start

Two spaces after dot in comments.

> +     of the clause list, but after any non-map clauses.  */
>    struct gimplify_adjust_omp_clauses_data data;
> -  data.list_p = list_p;
> +  tree *implicit_add_list_p = orig_list_p;
> +  while (*implicit_add_list_p
> +	 && OMP_CLAUSE_CODE (*implicit_add_list_p) != OMP_CLAUSE_MAP)
> +    implicit_add_list_p = &OMP_CLAUSE_CHAIN (*implicit_add_list_p);

Why are the implicit map clauses added first and not last?
There is also the OpenMP 5.1 [352:17-22] case which basically says that the
implicit mappings should be ignored if there are explicit ones on the same
construct (though, do we really create implicit clauses in that case?).

> --- a/include/gomp-constants.h
> +++ b/include/gomp-constants.h
> @@ -40,11 +40,22 @@
>  #define GOMP_MAP_FLAG_SPECIAL_0		(1 << 2)
>  #define GOMP_MAP_FLAG_SPECIAL_1		(1 << 3)
>  #define GOMP_MAP_FLAG_SPECIAL_2		(1 << 4)
> +#define GOMP_MAP_FLAG_SPECIAL_3		(1 << 5)
>  #define GOMP_MAP_FLAG_SPECIAL_4		(1 << 6)
>  #define GOMP_MAP_FLAG_SPECIAL		(GOMP_MAP_FLAG_SPECIAL_1 \
>  					 | GOMP_MAP_FLAG_SPECIAL_0)
>  #define GOMP_MAP_DEEP_COPY		(GOMP_MAP_FLAG_SPECIAL_4 \
>  					 | GOMP_MAP_FLAG_SPECIAL_2)
> +/* This value indicates the map was created implicitly according to
> +   OpenMP rules.  */
> +#define GOMP_MAP_IMPLICIT		(GOMP_MAP_FLAG_SPECIAL_3 \
> +					 | GOMP_MAP_FLAG_SPECIAL_4)
> +/* Mask for entire set of special map kind bits.  */
> +#define GOMP_MAP_FLAG_SPECIAL_BITS	(GOMP_MAP_FLAG_SPECIAL_0 \
> +					 | GOMP_MAP_FLAG_SPECIAL_1 \
> +					 | GOMP_MAP_FLAG_SPECIAL_2 \
> +					 | GOMP_MAP_FLAG_SPECIAL_3 \
> +					 | GOMP_MAP_FLAG_SPECIAL_4)
>  /* Flag to force a specific behavior (or else, trigger a run-time error).  */
>  #define GOMP_MAP_FLAG_FORCE		(1 << 7)
>  
> @@ -186,6 +197,9 @@ enum gomp_map_kind
>  #define GOMP_MAP_ALWAYS_P(X) \
>    (GOMP_MAP_ALWAYS_TO_P (X) || ((X) == GOMP_MAP_ALWAYS_FROM))
>  
> +#define GOMP_MAP_IMPLICIT_P(X) \
> +  (((X) & GOMP_MAP_FLAG_SPECIAL_BITS) == GOMP_MAP_IMPLICIT)

I think here we need to decide with which GOMP_MAP* kinds the implicit
bit will need to be combined with, with looking forward into what features
we still need to implement for OpenMP 5.0/5.1 (not aware of anything in 5.2
that would need special care but perhaps I've missed it).

E.g. for declare mapper those single OMP_CLAUSE_MAPs with the implicit
bit might need to be split into various smaller ones, map this FIELD_DECL,
map that other FIELD_DECL, what it points to, etc.  Even without declare
mapper, the spec now says that mapping a structure is as if each member is
mapped separately and 5.2 is going to say there is a predefined declare
mapper that does that (which is of course something we don't want under the
hood, we don't want thousands of maps, but virtually split it on a field by
field basis, do all the special stuff - e.g. how pointers are to be mapped
as zero array sections with pointer attachment, how C++ references are to be
handled etc. and then virtually coalesce all the adjacent fields that have
the same treatment back).  I think that actually means we should defer most
of the map struct etc. handling we do in gimplify_scan_omp_clauses, instead
note the explicit map clauses and what they refer to, add implicit ones (and
set implicit bit on those), then go over all clauses (explicit and
implicit), do the declare mapper processing, do the sorting of what that
produces based on base expressions/pointers etc.
At this point, I'm not sure if GOMP_MAP_IMPLICIT can or can't appear
together e.g. with GOMP_MAP_STRUCT.

> @@ -405,8 +422,24 @@ gomp_map_vars_existing (struct gomp_device_descr *devicep,
>  static int
>  get_kind (bool short_mapkind, void *kinds, int idx)
>  {
> -  return short_mapkind ? ((unsigned short *) kinds)[idx]
> -		       : ((unsigned char *) kinds)[idx];
> +  int val = (short_mapkind
> +	     ? ((unsigned short *) kinds)[idx]
> +	     : ((unsigned char *) kinds)[idx]);
> +
> +  if (GOMP_MAP_IMPLICIT_P (val))
> +    val &= ~GOMP_MAP_IMPLICIT;

As the particular bit isn't used for anything right now,
perhaps just do val &= ~GOMP_MAP_IMPLICIT unconditionally?
But, on the other side, for !short_mapkind you do not want
to mask that bit out, only the low 3 bits are the mapping
type and the upper bits are log2 of alignment, so the
above for !short_mapkind would in weird way change some
alignments.

> +  return val;
> +}
> +
> +
> +static bool
> +get_implicit (bool short_mapkind, void *kinds, int idx)
> +{
> +  int val = (short_mapkind
> +	     ? ((unsigned short *) kinds)[idx]
> +	     : ((unsigned char *) kinds)[idx]);
> +
> +  return GOMP_MAP_IMPLICIT_P (val);

Similarly can return 0 for !short_mapkind, the compatibility GOMP_target etc.
APIs will never have those in.

	Jakub