openacc reference reductions

[Jakub Jelinek]

On Fri, Apr 08, 2016 at 07:35:35AM -0700, Cesar Philippidis wrote:
> The FEs a little inconsistent, and I didn't want to make this patch that
> invasive. Can the FE changes wait to gcc7?

Sure.

> 2016-04-08  Cesar Philippidis  <cesar@codesourcery.com>
> 
> 	PR lto/70289
> 	PR ipa/70348
> 	PR tree-optimization/70373
> 	PR middle-end/70533
> 	PR middle-end/70534
> 	PR middle-end/70535
> 

No empty line between PR lines and * gimplify.c (... line.
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -7987,6 +7987,34 @@ gimplify_adjust_omp_clauses (gimple_seq *pre_p, gimple_seq body, tree *list_p,
>  	      break;
>  	    }
>  	  decl = OMP_CLAUSE_DECL (c);
> +	  /* Data clasues associated with acc parallel reductions must be
> +	     compatible with present_or_copy.  Warn and adjust the clause
> +	     if that is not the case.  */
> +	  if (ctx->region_type == ORT_ACC_PARALLEL)
> +	    {
> +	      tree t = DECL_P (decl) ? decl : TREE_OPERAND (decl, 0);
> +	      n = NULL;
> +
> +	      if (DECL_P (t))
> +		n = splay_tree_lookup (ctx->variables, (splay_tree_key)t);

There should be space before t.
> +
> +	      if (n && (n->value & GOVD_REDUCTION))
> +		{
> +		  int kind = OMP_CLAUSE_MAP_KIND (c);

Use gomp_map_kind or enum gomp_map_kind instead of int?

> +
> +		  OMP_CLAUSE_MAP_IN_REDUCTION(c) = 1;

Space before (.
> +		  if ((kind & GOMP_MAP_TOFROM) != GOMP_MAP_TOFROM
> +		      && kind != GOMP_MAP_FORCE_PRESENT
> +		      && kind != GOMP_MAP_POINTER)
> +		    {
> +		      warning_at (OMP_CLAUSE_LOCATION (c), 0,
> +				  "incompatible data clause with reduction "
> +				  "on %qE; promoting to present_or_copy",
> +				  DECL_NAME (t));
> +		      OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_TOFROM);
> +		    }
> +		}
> +	    }
>  	  if (!DECL_P (decl))
>  	    {
>  	      if ((ctx->region_type & ORT_TARGET) != 0
> @@ -8118,6 +8146,34 @@ gimplify_adjust_omp_clauses (gimple_seq *pre_p, gimple_seq body, tree *list_p,
>  
>  	case OMP_CLAUSE_REDUCTION:
>  	  decl = OMP_CLAUSE_DECL (c);
> +	  /* OpenACC reductions need a present_or_copy data clause.
> +	     Add one if necessary.  Error is the reduction is private.  */
> +	  if (ctx->region_type == ORT_ACC_PARALLEL)
> +	    {
> +	      n = splay_tree_lookup (ctx->variables, (splay_tree_key)decl);

Missing space.

> +	      if (n->value & (GOVD_PRIVATE | GOVD_FIRSTPRIVATE))
> +		{
> +		  error_at (OMP_CLAUSE_LOCATION (c), "invalid private "
> +			    "reduction on %qE", DECL_NAME (decl));
> +		}

Please avoid {}s around single statement.  Better don't break the
message into multiple lines in this case, so
		error_at (OMP_CLAUSE_LOCATION (c),
			  "invalid private reduction on %qE",
			  DECL_NAME (decl));
is more readable.

> +	      else if ((n->value & GOVD_MAP) == 0)
> +		{
> +		  tree next = OMP_CLAUSE_CHAIN (c);
> +		  tree nc = build_omp_clause (UNKNOWN_LOCATION, OMP_CLAUSE_MAP);

Too long line, please wrap.

> +		  OMP_CLAUSE_SET_MAP_KIND (nc, GOMP_MAP_TOFROM);
> +		  OMP_CLAUSE_DECL (nc) = decl;
> +		  OMP_CLAUSE_CHAIN (c) = nc;
> +		  lang_hooks.decls.omp_finish_clause (nc, pre_p);
> +		  for (; nc; nc = OMP_CLAUSE_CHAIN (nc))
> +		    {
> +		      OMP_CLAUSE_MAP_IN_REDUCTION (nc) = 1;
> +		      if (OMP_CLAUSE_CHAIN (nc) == NULL)
> +			break;
> +		    }

Then the nc; condition doesn't make sense.  Perhaps then
		  while (1)
		    {
		      OMP_CLAUSE_MAP_IN_REDUCTION (nc) = 1;
		      if (OMP_CLAUSE_CHAIN (nc) == NULL)
			break;
		      nc = OMP_CLAUSE_CHAIN (nc);
		    }
or
		  for (; ; nc = OMP_CLAUSE_CHAIN (nc))
		    {
		      OMP_CLAUSE_MAP_IN_REDUCTION (nc) = 1;
		      if (OMP_CLAUSE_CHAIN (nc) == NULL)
			break;
		    }
?

> @@ -5624,22 +5625,38 @@ lower_oacc_reductions (location_t loc, tree clauses, tree level, bool inner,
> +		  else if ((OMP_CLAUSE_CODE (cls) == OMP_CLAUSE_FIRSTPRIVATE
> +			    || OMP_CLAUSE_CODE (cls) == OMP_CLAUSE_PRIVATE)
> +			   && orig == OMP_CLAUSE_DECL (cls))
> +		    {
> +		      is_private = true;
> +		      goto do_lookup;
> +		    }

Isn't this case rejected by the gimplifier?

> @@ -15829,7 +15874,10 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, omp_context *ctx)
>  	if (!maybe_lookup_field (var, ctx))
>  	  continue;
>  
> -	if (offloaded)
> +	/* Don't remap oacc parallel reduction variables, because the
> +	   intermediate result must be local to each gang.  */
> +	if (offloaded && !(OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
> +			   && OMP_CLAUSE_MAP_IN_REDUCTION(c)))

Missing space before after OMP_CLAUSE_MAP_IN_REDUCTION

Ok for trunk with those changes if the lower_oacc_reduction is_private
handling is still needed, if it is not needed, please clean that up.

	Jakub