On 06/04/2014 12:49 PM, Thomas Schwinge wrote:
(Note that I split up the original patches into two components. This is
the omp-low.c component.)
> 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.
You're correct, it does require more work. The way that the loop clause
is handle in fortran is that all loops get lowered with the collapse
clause set. By default, for non-concurrent loops, collapse is set to 1.
And when collapse == 1, nothing special happens during the omp-lowering
phase.
In this updated patch, I removed the c front end changes. Also, collapse
support in fortran is restricted to collapse(1) or else the do loop
clause will do nothing. Any collapse value != 1 will get caught by one
of the existing asserts. I've got a more complete collapse patch, but
it's not thoroughly tested yet.
> (Also, I'm not sure to
> which extent we're at all currently handling combined directives in
> gimplification and lowering?)
Do you mean something like
$!acc parallel loop
? That doesn't work yet. But it does work when you separate them.
If this patch is OK, please commit it for me.
Thanks,
Cesar
>> + 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
>
2014-06-04 Cesar Philippidis <ce...@codesourcery.com>
* gcc/omp-low.c (scan_sharing_clauses): Shuffle OMP_CLAUSE_COLLAPSE
and OMP_CLAUSE_PRIVATE around to enable in openacc.
diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index 3e282c0..05df6ee 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -1534,7 +1534,6 @@ 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));
decl = OMP_CLAUSE_DECL (c);
if (OMP_CLAUSE_PRIVATE_OUTER_REF (c))
goto do_private;
@@ -1762,15 +1761,18 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
}
}
break;
-
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));
+ if (is_gimple_omp_oacc_specifically (ctx->stmt))
+ {
+ sorry ("clause not supported yet");
+ break;
+ }
+ case OMP_CLAUSE_COLLAPSE:
break;
case OMP_CLAUSE_ALIGNED:
@@ -1814,7 +1816,6 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
break;
/* FALLTHRU */
- case OMP_CLAUSE_PRIVATE:
case OMP_CLAUSE_FIRSTPRIVATE:
case OMP_CLAUSE_REDUCTION:
if (is_gimple_omp_oacc_specifically (ctx->stmt))
@@ -1824,6 +1825,7 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
}
case OMP_CLAUSE_LINEAR:
gcc_assert (!is_gimple_omp_oacc_specifically (ctx->stmt));
+ case OMP_CLAUSE_PRIVATE:
decl = OMP_CLAUSE_DECL (c);
if (is_variable_sized (decl))
install_var_local (decl, ctx);
@@ -1907,7 +1909,6 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
case OMP_CLAUSE_DIST_SCHEDULE:
case OMP_CLAUSE_NOWAIT:
case OMP_CLAUSE_ORDERED:
- case OMP_CLAUSE_COLLAPSE:
case OMP_CLAUSE_UNTIED:
case OMP_CLAUSE_FINAL:
case OMP_CLAUSE_MERGEABLE:
@@ -1919,6 +1920,7 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
case OMP_CLAUSE_TO:
case OMP_CLAUSE_FROM:
gcc_assert (!is_gimple_omp_oacc_specifically (ctx->stmt));
+ case OMP_CLAUSE_COLLAPSE:
case OMP_CLAUSE_NUM_GANGS:
case OMP_CLAUSE_NUM_WORKERS:
case OMP_CLAUSE_VECTOR_LENGTH:
