Re: [OpenACC] declare directive

[James Norris]

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