On Wed, Apr 06, 2016 at 01:21:30PM -0700, Cesar Philippidis wrote:
> That's a good idea. I went ahead and combined this patch with the data
> map reduction fix for PR70289 that I posted on Monday,
> <https://gcc.gnu.org/ml/gcc-patches/2016-04/msg00202.html>, because I'm
> already scanning for parallel reduction data clauses in there. As you
> suggested, I introduced an OMP_CLAUSE_MAP_IN_REDUCTION macro to the data
> clauses associated with acc parallel reductions.
>
> Is this patch OK for trunk? It fixes PR70289, PR70348, PR70373, PR70533,
> PR70535 and PR70537.
> 2016-04-06 Cesar Philippidis <ce...@codesourcery.com>
>
> PR lto/70289
Then please use
PR lto/70289
PR ipa/70348
PR tree-optimization/70373
PR middle-end/70533
PR middle-end/70535
PR70537 sounds like a typo to me, did you mean some other PR?
> gcc/
> * gimplify.c (gimplify_adjust_acc_parallel_reductions): New function.
...
> * gcc/tree.h (OMP_CLAUSE_MAP_IN_REDUCTION): New macro.
No gcc/ prefix please.
> * testsuite/libgomp.oacc-c-c++-common/reduction-1.c: Increate test
> coverage.
Increase?
> * testsuite/libgomp.oacc-c-c++-common/reduction-2.c: Likewise.
> * testsuite/libgomp.oacc-c-c++-common/reduction-3.c: Likewise.
> * testsuite/libgomp.oacc-c-c++-common/reduction-4.c: Likewise.
> * testsuite/libgomp.oacc-c-c++-common/reduction-5.c: Likewise.
> * testsuite/libgomp.oacc-c-c++-common/reduction-6.c: New test.
> * testsuite/libgomp.oacc-c-c++-common/reduction.h: New test.
> * testsuite/libgomp.oacc-fortran/parallel-reduction.f90: New test.
> * testsuite/libgomp.oacc-fortran/pr70289.f90: New test.
> * testsuite/libgomp.oacc-fortran/reduction-1.f90: Increate test
Ditto.
> + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_PRIVATE
> + || OMP_CLAUSE_CODE (c) == OMP_CLAUSE_FIRSTPRIVATE)
> + {
> + error_at (OMP_CLAUSE_LOCATION (c), "invalid private reduction "
> + "on %qE", DECL_NAME (decl));
This looks wrongly formatted, "on is not below OMP.
> + /* Scan 3: Add a present_or_copy clause for any reduction variable which
> + doens't have a data clause already. */
doesn't
> + for (hash_set<tree>::iterator iter = reduction_decls->begin ();
> + iter != reduction_decls->end (); ++iter)
> + {
> + tree decl = *iter;
> +
> + tree nc = build_omp_clause (UNKNOWN_LOCATION, OMP_CLAUSE_MAP);
> + OMP_CLAUSE_SET_MAP_KIND (nc, GOMP_MAP_TOFROM);
> + OMP_CLAUSE_DECL (nc) = decl;
> + if (!POINTER_TYPE_P (TREE_TYPE (decl)))
> + OMP_CLAUSE_MAP_IN_REDUCTION (nc) = 1;
> + TREE_CHAIN (nc) = list;
> + list = nc;
> + }
> +
> + cleanup:
> + delete reduction_decls;
> + delete pointer_decls;
> +
> + return list;
> +}
But more importantly, do you really have to do this separately?
I admit I haven't stepped through your testcases in the debugger, so
correct me if I'm missing something:
I mean, gimplify_scan_omp_clauses should omp_add_variable for the
OMP_CLAUSE_REDUCTION with
GOVD_REDUCTION | GOVD_SEEN | GOVD_EXPLICIT
and OMP_CLAUSE_MAP with GOVD_MAP | GOVD_EXPLICIT or so, similarly
GOVD_PRIVATE and/or GOVD_FIRSTPRIVATE flags from
OMP_CLAUSE_PRIVATE/OMP_CLAUSE_FIRSTPRIVATE.
So I believe you should have all the info you need in
(gimplify_adjust_omp_clauses) <case OMP_CLAUSE_REDUCTION>,
you have the CODE of the construct this is on (so check OACC_PARALLEL
or whatever you need), and you should be able to check if there is
explicit map/private/firstprivate clause together with OMP_CLAUSE_REDUCTION
or not, and then you can add the extra implicit clause and set
OMP_CLAUSE_MAP_IN_REDUCTION on it as appropriate.
Jakub
