Re: [OpenACC] declare directive

[Jakub Jelinek <jakub at redhat dot com>]

On Mon, Nov 09, 2015 at 10:01:32AM -0600, James Norris wrote:
> +      if (lookup_attribute ("omp declare target", DECL_ATTRIBUTES (decl)))

Here you only look up "omp declare target", not "omp declare target link".
So, what happens if you mix that (once in some copy clause, once in link),
or mention twice in link, etc.?  Needs testsuite coverage and clear rules.

> +	  DECL_ATTRIBUTES (decl) =
> +			tree_cons (id, NULL_TREE, DECL_ATTRIBUTES (decl));

Incorrect formatting, = goes already on the following line, no whitespace
at end of line, and next line is indented below CL from DECL.

> +	  t = build_omp_clause (OMP_CLAUSE_LOCATION (c) , OMP_CLAUSE_MAP);

Wrong formatting, no space before ,.

> +    if (ret_clauses)
> +      {
> +	tree fndecl = current_function_decl;
> +	tree attrs = lookup_attribute ("oacc declare returns",
> +				       DECL_ATTRIBUTES (fndecl));

Why do you use an attribute for this?  I think adding the automatic
vars to hash_map during gimplification of the OACC_DECLARE is best.

> +	    tree id = get_identifier ("oacc declare returns");
> +	    DECL_ATTRIBUTES (fndecl) =
> +			tree_cons (id, ret_clauses, DECL_ATTRIBUTES (fndecl));

Formatting error.

> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -1065,6 +1065,7 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
>    gimple_seq body, cleanup;
>    gcall *stack_save;
>    location_t start_locus = 0, end_locus = 0;
> +  tree ret_clauses = NULL;
>  
>    tree temp = voidify_wrapper_expr (bind_expr, NULL);
>  
> @@ -1166,9 +1167,56 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
>  	  clobber_stmt = gimple_build_assign (t, clobber);
>  	  gimple_set_location (clobber_stmt, end_locus);
>  	  gimplify_seq_add_stmt (&cleanup, clobber_stmt);
> +
> +	  if (flag_openacc)
> +	    {
> +	      tree attrs = lookup_attribute ("oacc declare returns",
> +				       DECL_ATTRIBUTES (current_function_decl));
> +	      tree clauses, c, c_next = NULL, c_prev = NULL;
> +
> +	      if (!attrs)
> +		break;
> +
> +	      clauses = TREE_VALUE (attrs);
> +
> +	      for (c = clauses; c; c_prev = c, c = c_next)
> +		{
> +		  c_next = OMP_CLAUSE_CHAIN (c);
> +
> +		  if (t == OMP_CLAUSE_DECL (c))
> +		    {
> +		      if (ret_clauses)
> +			OMP_CLAUSE_CHAIN (c) = ret_clauses;
> +
> +		      ret_clauses = c;
> +
> +		      if (c_prev == NULL)
> +			clauses = c_next;
> +		      else
> +			OMP_CLAUSE_CHAIN (c_prev) = c_next;
> +		    }
> +		}

This doesn't really scale.  Consider 10000 clauses on various
oacc declare constructs in a single function, and 1000000 automatic
variables in such a function.
So, what I'm suggesting is during gimplification of OACC_DECLARE,
if you find a clause on an automatic variable in the current function
that you want to unmap afterwards, have a
static hash_map<tree, tree> *oacc_declare_returns;
and you just add into the hash map the VAR_DECL -> the clause you want,
then in this spot you check
  if (oacc_declare_returns)
    {
      clause = lookup in hash_map (t);
      if (clause)
	{
	  ...
	}
    }

> +
> +	      if (clauses == NULL)
> +		{
> +		  DECL_ATTRIBUTES (current_function_decl) =
> +			    remove_attribute ("oacc declare returns",
> +				    DECL_ATTRIBUTES (current_function_decl));

Wrong formatting.

	Jakub