[OpenACC] declare directive

James Norris jnorris@codesourcery.com
Mon Nov 9 16:31:00 GMT 2015


Jakub,

On 11/09/2015 10:21 AM, Jakub Jelinek wrote:
> 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.

Will fix.

>
>> +	  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.

Will fix.

>
>> +	  t = build_omp_clause (OMP_CLAUSE_LOCATION (c) , OMP_CLAUSE_MAP);
> Wrong formatting, no space before ,.

Will fix.

>> +    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.

See below (This doesn't scale...)

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

Will fix.

>
>> --- 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)
> 	{
> 	  ...
> 	}
>      }

Now I see what you were getting at in using the hash_map. I didn't
consider creating a static hash_map and populating it as you suggest.

Thank you!

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

Will fix.

>
> 	Jakub

Thanks for taking the time to review.

Jim




More information about the Gcc-patches mailing list