[PATCH, 2/3, OpenMP] Target mapping changes for OpenMP 5.0, middle-end parts and compiler testcases

[Jakub Jelinek]

On Tue, Sep 01, 2020 at 09:16:48PM +0800, Chung-Lin Tang wrote:
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -8350,14 +8350,126 @@ extract_base_bit_offset (tree base, tree *base_ref, poly_int64 *bitposp,
>    /* Set *BASE_REF if BASE was a dereferenced reference variable.  */
>    if (base_ref && orig_base != base)
>      *base_ref = orig_base;
>  
>    return base;
>  }
>  
> +/* Returns true if EXPR is or contains (as a sub-component) BASE_PTR.  */
> +
> +static bool
> +is_or_contains_p (tree expr, tree base_ptr)
> +{
> +  while (expr != base_ptr)
> +    if (TREE_CODE (base_ptr) == COMPONENT_REF)
> +      base_ptr = TREE_OPERAND (base_ptr, 0);
> +    else
> +      break;
> +  return expr == base_ptr;
> +}
> +
> +/* Implement OpenMP 5.x map ordering rules for target directives. There are
> +   several rules, and with some level of ambiguity, hopefully we can at least
> +   collect the complexity here in one place.  */
> +
> +static void
> +omp_target_reorder_clauses (tree *list_p)
> +{

So, first of all, are you convinced we can sort just the explicit clauses
and leave out the (later on) implicitly added ones?
If it is possible, sure, it will be easier (because we later on need to deal
with the GOMP_MAP_STRUCT sorting too).

> +  vec<tree> clauses = vNULL;

Isn't this a memory leak?  Nothing frees the vector.  Perhaps better
  auto_vec<tree, 32> clauses;

> +  for (tree *cp = list_p; *cp; cp = &OMP_CLAUSE_CHAIN (*cp))
> +    clauses.safe_push (*cp);

The rest of the function deals only with OMP_CLAUSE_MAP clauses, wouldn't it
be better to just push to the vec those clauses and keep other clauses just
in *list_p chain?

> +  /* Collect refs to alloc/release/delete maps.  */
> +  vec<tree> ard = vNULL;

Again, auto_vec<tree, 32> ard;

> +  tree *cp = list_p;
> +  for (unsigned int i = 0; i < clauses.length (); i++)
> +    if (clauses[i])
> +      {
> +	*cp = clauses[i];
> +	cp = &OMP_CLAUSE_CHAIN (clauses[i]);
> +      }
> +  for (unsigned int i = 0; i < ard.length (); i++)
> +    {
> +      *cp = ard[i];
> +      cp = &OMP_CLAUSE_CHAIN (ard[i]);
> +    }
> +  *cp = NULL_TREE;
> +
> +  /* OpenMP 5.0 requires that pointer variables are mapped before
> +     its use as a base-pointer.  */
> +  for (tree *cp = list_p; *cp; cp = &OMP_CLAUSE_CHAIN (*cp))
> +    if (OMP_CLAUSE_CODE (*cp) == OMP_CLAUSE_MAP)
> +      {
> +	tree decl = OMP_CLAUSE_DECL (*cp);
> +	gomp_map_kind k = OMP_CLAUSE_MAP_KIND (*cp);
> +	if ((k == GOMP_MAP_ALLOC
> +	     || k == GOMP_MAP_TO
> +	     || k == GOMP_MAP_FROM
> +	     || k == GOMP_MAP_TOFROM)

What about the *ALWAYS* kinds?

> +	    && (TREE_CODE (decl) == INDIRECT_REF
> +		|| TREE_CODE (decl) == MEM_REF))
> +	  {
> +	    tree base_ptr = TREE_OPERAND (decl, 0);
> +	    STRIP_TYPE_NOPS (base_ptr);
> +	    for (tree *cp2 = &OMP_CLAUSE_CHAIN (*cp); *cp2;
> +		 cp2 = &OMP_CLAUSE_CHAIN (*cp2))
> +	      if (OMP_CLAUSE_CODE (*cp2) == OMP_CLAUSE_MAP)
> +		{
> +		  tree decl2 = OMP_CLAUSE_DECL (*cp2);
> +		  gomp_map_kind k2 = OMP_CLAUSE_MAP_KIND (*cp2);
> +		  if ((k2 == GOMP_MAP_ALLOC
> +		       || k2 == GOMP_MAP_TO
> +		       || k2 == GOMP_MAP_FROM
> +		       || k2 == GOMP_MAP_TOFROM)

Again.

This is O(n^2) too, but due to the is_or_contains_p I'm not sure
if we can avoid it.  Perhaps sort the clauses by uid of the base expressions
and deal with those separately.  Maybe let's ignore it for now.

> @@ -8958,25 +9083,20 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p,
>  	      /* An "attach/detach" operation on an update directive should
>  		 behave as a GOMP_MAP_ALWAYS_POINTER.  Beware that
>  		 unlike attach or detach map kinds, GOMP_MAP_ALWAYS_POINTER
>  		 depends on the previous mapping.  */
>  	      if (code == OACC_UPDATE
>  		  && OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH_DETACH)
>  		OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_ALWAYS_POINTER);
> -	      if (gimplify_expr (pd, pre_p, NULL, is_gimple_lvalue, fb_lvalue)
> -		  == GS_ERROR)
> -		{
> -		  remove = true;
> -		  break;
> -		}

So what gimplifies those now?

> +			    if (! (code == OMP_TARGET
> +				   || code == OMP_TARGET_DATA
> +				   || code == OMP_TARGET_ENTER_DATA
> +				   || code == OMP_TARGET_EXIT_DATA))
> +			      {

Isn't this just if ((region_type & ORT_ACC) == 0) ?  Or do we want
it for target update too?  Though, we wouldn't talk about more than once in
map clauses then because target update doesn't have those.


	Jakub