Re: [PATCH, OpenACC] (1/2) Fix implicit mapping for array slices on lexically-enclosing data constructs (PR70828)

[Jakub Jelinek <jakub at redhat dot com>]

On Tue, Aug 28, 2018 at 03:19:19PM -0400, Julian Brown wrote:
> 2018-08-28  Julian Brown  <julian@codesourcery.com>
> 	    Cesar Philippidis  <cesar@codesourcery.com>
> 
> 	PR middle-end/70828
> 
> 	gcc/
> 	* gimplify.c (gimplify_omp_ctx): Add decl_data_clause hash map.
> 	(new_omp_context): Initialise above.
> 	(delete_omp_context): Delete above.
> 	(gimplify_scan_omp_clauses): Scan for array mappings on data constructs,
> 	and record in above map.
> 	(gomp_needs_data_present): New function.
> 	(gimplify_adjust_omp_clauses_1): Handle data mappings (e.g. array
> 	slices) declared in lexically-enclosing data constructs.
> 	* omp-low.c (lower_omp_target): Allow decl for bias not to be present
> 	in omp context.
> 	
> 	gcc/testsuite/
> 	* c-c++-common/goacc/acc-data-chain.c: New test.
> 	* gfortran.dg/goacc/pr70828.f90: New test.
> 	* gfortran.dg/goacc/pr70828-2.f90: New test.
> 
> 	libgomp/
> 	* testsuite/libgomp.oacc-c-c++-common/pr70828.c: New test.
> 	* testsuite/libgomp.oacc-fortran/implicit_copy.f90: New test.
> 	* testsuite/libgomp.oacc-fortran/pr70828.f90: New test.
> 	* testsuite/libgomp.oacc-fortran/pr70828-2.f90: New test.
> 	* testsuite/libgomp.oacc-fortran/pr70828-3.f90: New test.
> 	* testsuite/libgomp.oacc-fortran/pr70828-5.f90: New test.

> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -191,6 +191,7 @@ struct gimplify_omp_ctx
>    bool target_map_scalars_firstprivate;
>    bool target_map_pointers_as_0len_arrays;
>    bool target_firstprivatize_array_bases;
> +  hash_map<tree, std::pair<tree, tree> > *decl_data_clause;
>  };
>  
>  static struct gimplify_ctx *gimplify_ctxp;
> @@ -413,6 +414,7 @@ new_omp_context (enum omp_region_type region_type)
>      c->default_kind = OMP_CLAUSE_DEFAULT_SHARED;
>    else
>      c->default_kind = OMP_CLAUSE_DEFAULT_UNSPECIFIED;
> +  c->decl_data_clause = new hash_map<tree, std::pair<tree, tree> >;

Not really happy about creating this unconditionally.  Can you leave it
NULL by default and only initialize for contexts where it will be needed?

> @@ -7793,8 +7796,21 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p,
>  	    case OMP_TARGET:
>  	      break;
>  	    case OACC_DATA:
> -	      if (TREE_CODE (TREE_TYPE (decl)) != ARRAY_TYPE)
> -		break;
> +	      {
> +		tree nextc = OMP_CLAUSE_CHAIN (c);
> +		if (nextc
> +		    && OMP_CLAUSE_CODE (nextc) == OMP_CLAUSE_MAP
> +		    && (OMP_CLAUSE_MAP_KIND (nextc)
> +			  == GOMP_MAP_FIRSTPRIVATE_POINTER
> +			|| OMP_CLAUSE_MAP_KIND (nextc) == GOMP_MAP_POINTER))
> +		  {
> +	            tree base_addr = OMP_CLAUSE_DECL (nextc);
> +		    ctx->decl_data_clause->put (base_addr,
> +		      std::make_pair (unshare_expr (c), unshare_expr (nextc)));

Don't like the wrapping here, can you just split it up:
		    std::pair<tree, tree> p
		      = std::make_pair (unshare_expr (c),
					unshare_expr (nextc));
		    ctx->decl_data_clause->put (base_addr, p);
or similar?

> +
> +static std::pair<tree, tree> *
> +gomp_needs_data_present (tree decl)

Would be helpful to have acc/oacc in the function name.
> +{
> +  gimplify_omp_ctx *ctx = NULL;
> +
> +  if (TREE_CODE (TREE_TYPE (decl)) != ARRAY_TYPE
> +      && TREE_CODE (TREE_TYPE (decl)) != POINTER_TYPE
> +      && (TREE_CODE (TREE_TYPE (decl)) != POINTER_TYPE
> +	  || TREE_CODE (TREE_TYPE (TREE_TYPE (decl))) != ARRAY_TYPE))
> +    return NULL;
> +
> +  if (gimplify_omp_ctxp->region_type != ORT_ACC_PARALLEL
> +      && gimplify_omp_ctxp->region_type != ORT_ACC_KERNELS)
> +    return NULL;

And move this test to the top.

> --- a/gcc/omp-low.c
> +++ b/gcc/omp-low.c
> @@ -8411,9 +8411,10 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, omp_context *ctx)
>  		x = fold_convert_loc (clause_loc, type, x);
>  		if (!integer_zerop (OMP_CLAUSE_SIZE (c)))
>  		  {
> -		    tree bias = OMP_CLAUSE_SIZE (c);
> -		    if (DECL_P (bias))
> -		      bias = lookup_decl (bias, ctx);
> +		    tree bias = OMP_CLAUSE_SIZE (c), remapped_bias;
> +		    if (DECL_P (bias)
> +			&& (remapped_bias = maybe_lookup_decl (bias, ctx)))
> +		      bias = remapped_bias;
>  		    bias = fold_convert_loc (clause_loc, sizetype, bias);
>  		    bias = fold_build1_loc (clause_loc, NEGATE_EXPR, sizetype,
>  					    bias);

This is shared with OpenMP and must be conditionalized for OpenACC only.

	Jakub