Hi Cesar! On Tue, 3 Jun 2014 16:00:30 -0700, Cesar Philippidis <ce...@codesourcery.com> wrote: > in order to make > the patch yield more interesting results, I've also enabled the private > clause. Is this patch ok for the gomp-4_0-branch?
> gcc/
> * c/c-parser.c (c_parser_oacc_all_clauses): Update handling for
Note that gcc/c/ as well as gcc/fortran/ have separate ChangeLog* files.
> OMP_CLAUSE_COLLAPSE and OMP_CLAUSE_PRIVATE.
Only for OMP_CLAUSE_COLLAPSE, not OMP_CLAUSE_PRIVATE.
> (c_parser_oacc_kernels): Likewise.
OACC_LOOP_CLAUSE_MASK, not c_parser_oacc_kernels.
> --- a/gcc/c/c-parser.c
> +++ b/gcc/c/c-parser.c
> @@ -11228,6 +11228,10 @@ c_parser_oacc_all_clauses (c_parser *parser,
> omp_clause_mask mask,
>
> switch (c_kind)
> {
> + case PRAGMA_OMP_CLAUSE_COLLAPSE:
Won't this need additional work? It seems that for combined directives
(kernels loop, parallel loop), we currently don't (or, don't correctly)
parse the clauses, and support in clause splitting
(c-family/c-omp:c_omp_split_clauses) is also (generally) missing, I
think? Anyway, this is a separate change from your Fortran loop support,
so should (ideally) be a separate patch/commit. (Also, I'm not sure to
which extent we're at all currently handling combined directives in
gimplification and lowering?)
> + clauses = c_parser_omp_clause_collapse (parser, clauses);
> + c_name = "collapse";
> + break;
Update the comment on c_parser_omp_clause_collapse to state that it's for
OpenACC, too.
> @@ -12217,8 +12221,8 @@ c_parser_omp_for_loop (location_t loc, c_parser
> *parser, enum tree_code code,
> for (cl = clauses; cl; cl = OMP_CLAUSE_CHAIN (cl))
> if (OMP_CLAUSE_CODE (cl) == OMP_CLAUSE_COLLAPSE)
> {
> - gcc_assert (code != OACC_LOOP);
> - collapse = tree_to_shwi (OMP_CLAUSE_COLLAPSE_EXPR (cl));
> + //gcc_assert (code != OACC_LOOP);
> + collapse = tree_to_shwi (OMP_CLAUSE_COLLAPSE_EXPR (cl));
> }
As Ilmir noted, remove the gcc_assert -- assuming you have some
confidence that the following code (including gimplification and
lowering) matches the OpenACC semantics for collapse != 1.
> --- a/gcc/gimple.h
> +++ b/gcc/gimple.h
> @@ -5809,15 +5809,25 @@ is_gimple_omp (const_gimple stmt)
> need any special handling for OpenACC. */
>
> static inline bool
> -is_gimple_omp_oacc_specifically (const_gimple stmt)
> +is_gimple_omp_oacc_specifically (const_gimple stmt,
> + enum omp_clause_code code = OMP_CLAUSE_ERROR)
> {
> gcc_assert (is_gimple_omp (stmt));
> switch (gimple_code (stmt))
> {
> case GIMPLE_OACC_KERNELS:
> case GIMPLE_OACC_PARALLEL:
> - return true;
> + switch (code)
> + {
> + case OMP_CLAUSE_COLLAPSE:
> + case OMP_CLAUSE_PRIVATE:
> + return false;
> + default:
> + return true;
> + }
> case GIMPLE_OMP_FOR:
> + if (code == OMP_CLAUSE_COLLAPSE || code == OMP_CLAUSE_PRIVATE)
> + return false;
> switch (gimple_omp_for_kind (stmt))
> {
> case GF_OMP_FOR_KIND_OACC_LOOP:
Hmm, why do we need this?
> --- a/gcc/omp-low.c
> +++ b/gcc/omp-low.c
> @@ -1534,7 +1534,8 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
> switch (OMP_CLAUSE_CODE (c))
> {
> case OMP_CLAUSE_PRIVATE:
> - gcc_assert (!is_gimple_omp_oacc_specifically (ctx->stmt));
> + gcc_assert (!is_gimple_omp_oacc_specifically (ctx->stmt,
> + OMP_CLAUSE_CODE (c)));
I'd say, in these "guarded" code paths, if you have confidence that
they're now correct for OpenACC, that is, a clause such as
OMP_CLAUSE_PRIVATE is "interpreted" correctly for OpenACC (it has the
same semantics as as for OpenMP), then you should simply remove the
assert completely (or, if applicable, move the case OMP_CLAUSE_PRIVATE or
the surrounding cases so that OMP_CLAUSE_PRIVATE is no longer covered by
the assert). For example, do it like this:
case OMP_CLAUSE_NOWAIT:
case OMP_CLAUSE_ORDERED:
- case OMP_CLAUSE_COLLAPSE:
case OMP_CLAUSE_UNTIED:
case OMP_CLAUSE_MERGEABLE:
case OMP_CLAUSE_PROC_BIND:
case OMP_CLAUSE_SAFELEN:
gcc_assert (!is_gimple_omp_oacc_specifically (ctx->stmt));
+ /* FALLTHRU */
+ case OMP_CLAUSE_COLLAPSE:
break;
With these things addressed/verified, the OMP_CLAUSE_COLLAPSE changes are
good to commit, thanks!
> @@ -1762,7 +1763,6 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
> }
> }
> break;
> -
> case OMP_CLAUSE_NOWAIT:
> case OMP_CLAUSE_ORDERED:
> case OMP_CLAUSE_COLLAPSE:
To ease my life ;-) as a branch maintainer, please don't introduce such
divergence from the GCC trunk code.
> @@ -1817,13 +1818,9 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
> case OMP_CLAUSE_PRIVATE:
> case OMP_CLAUSE_FIRSTPRIVATE:
> case OMP_CLAUSE_REDUCTION:
> - if (is_gimple_omp_oacc_specifically (ctx->stmt))
> - {
> - sorry ("clause not supported yet");
> - break;
> - }
Above that block is OMP_CLAUSE_LASTPRIVATE, which should have (should get
added) an assert for !OpenACC, and even though we're adding the OpenACC
private, firstprivate, and reduction clauses, we're not there yet; the
OpenACC private and firstprivate ones do differ from the OpenMP ones; I
have a WIP patch. (And, unless I'm confused, there even is a difference
in OpenACC depending on whether the private clause is attached to
parallel or loop directive... Wonder how that is to work with the
combined parallel loop directive?)
Ilmir says you're then getting an ICE instead of this sorry message; in
this case it's probably indeed better to keep the sorry message for the
respective unsupported clauses.
> @@ -1896,6 +1893,7 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
> sorry ("clause not supported yet");
> break;
> }
> + break;
> case OMP_CLAUSE_COPYPRIVATE:
> case OMP_CLAUSE_COPYIN:
> case OMP_CLAUSE_DEFAULT:
No need for that break, I think?
So, if this helps you to make progress, I'm OK for you to commit the
preliminary support for OMP_CLAUSE_PRIVATE, and I'll then revisit this
clause/code in the near future, for the correct OpenACC semantics.
Review of the Fortran changes I'll defer to someone who knows this code
(thanks already, Ilmir!); only one small comment:
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/goacc/loop-tree.f95
> +[...]
> \ No newline at end of file
Please add that one. ;-)
Grüße,
Thomas
pgpI1cD3QRptZ.pgp
Description: PGP signature
