diff mbox

[gomp4] openacc loops

Message ID 538FE718.9040400@codesourcery.com
State New
Headers show

Commit Message

Cesar Philippidis June 5, 2014, 3:42 a.m. UTC 
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 <cesar@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
>
diff mbox

Patch

2014-06-04  Cesar Philippidis  <cesar@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: