Re: OpenACC middle end changes

[Thomas Schwinge <thomas at codesourcery dot com>]

Hi Jakub!

Many thanks for the review comments!  The very most have been addresed,
here are just a few comments.  If you feel strongly/differently about
any, I'll address those, too.


On Thu, 13 Nov 2014 19:09:49 +0100, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Nov 13, 2014 at 05:59:11PM +0100, Thomas Schwinge wrote:
> > --- gcc/gimple-pretty-print.c
> > +++ gcc/gimple-pretty-print.c
> > @@ -1136,18 +1136,21 @@ dump_gimple_omp_for (pretty_printer *buffer, gimple gs, int spc, int flags)
> >  	case GF_OMP_FOR_KIND_FOR:
> >  	  kind = "";
> >  	  break;
> > -	case GF_OMP_FOR_KIND_SIMD:
> > -	  kind = " simd";
> > -	  break;
> > -	case GF_OMP_FOR_KIND_CILKSIMD:
> > -	  kind = " cilksimd";
> > -	  break;
> >  	case GF_OMP_FOR_KIND_DISTRIBUTE:
> >  	  kind = " distribute";
> >  	  break;
> >  	case GF_OMP_FOR_KIND_CILKFOR:
> >  	  kind = " _Cilk_for";
> >  	  break;
> > +	case GF_OMP_FOR_KIND_OACC_LOOP:
> > +	  kind = " oacc_loop";
> > +	  break;
> > +	case GF_OMP_FOR_KIND_SIMD:
> > +	  kind = " simd";
> > +	  break;
> > +	case GF_OMP_FOR_KIND_CILKSIMD:
> > +	  kind = " cilksimd";
> > +	  break;
> 
> Why the reshuffling?  The result isn't alphabetically sorted
> anyway.  I'd just add new stuff at the end ;)

It's the order in which the GF_OMP_FOR_KIND_* are defined.  At least for
my mind ;-) that makes it very easy to grasp that all of them are
covered.


> > @@ -1684,7 +1718,12 @@ gimple_copy (gimple stmt)
> >  	  gimple_try_set_cleanup (copy, new_seq);
> >  	  break;
> >  
> > +	case GIMPLE_OACC_KERNELS:
> > +	case GIMPLE_OACC_PARALLEL:
> > +          gcc_unreachable ();
> > +
> >  	case GIMPLE_OMP_FOR:
> > +	  gcc_assert (!is_gimple_omp_oacc_specifically (stmt));
> >  	  new_seq = gimple_seq_copy (gimple_omp_for_pre_body (stmt));
> >  	  gimple_omp_for_set_pre_body (copy, new_seq);
> >  	  t = unshare_expr (gimple_omp_for_clauses (stmt));
> > @@ -1754,6 +1793,7 @@ gimple_copy (gimple stmt)
> >  	case GIMPLE_OMP_TASKGROUP:
> >  	case GIMPLE_OMP_ORDERED:
> >  	copy_omp_body:
> > +	  gcc_assert (!is_gimple_omp_oacc_specifically (stmt));
> >  	  new_seq = gimple_seq_copy (gimple_omp_body (stmt));
> >  	  gimple_omp_set_body (copy, new_seq);
> >  	  break;
> 
> Why are you so afraid to support oacc in gimple_copy?

Not afraid, but have never run into this, so didn't want to claim such
code paths have been tested.  Anyway, with GIMPLE_OACC_* now merged into
GIMPLE_OMP_TARGET, this should -- famous last words?  ;-) -- now just
work, so I removed all these asserts.  (And, no doubt, usage of
assert/gcc_unreachable was not exactly correct, for code paths that could
actually legitimately be reached; probably should've used sorry instead.)


> > +/* GIMPLE_OACC_PARALLEL */
> > +struct GTY((tag("GSS_OMP_PARALLEL_LAYOUT")))
> > +  gimple_statement_oacc_parallel : public gimple_statement_omp_parallel_layout
> > +{
> > +    /* No extra fields; adds invariant:
> > +         stmt->code == GIMPLE_OACC_PARALLEL.  */
> 
> Formatting, there is too big indentation.  That said, this will be replaced
> with GF_OMP_TARGET_KIND_*, right?
> 
> > +static inline tree
> > +gimple_oacc_kernels_clauses (const_gimple gs)
> > +{
> > +  const gimple_statement_oacc_kernels *oacc_kernels_stmt =
> > +    as_a <const gimple_statement_oacc_kernels *> (gs);
> 
> Wrong formatting (many times, = goes on the second line.
> Seems you probably just copied it from preexisting bad formatting,
> we should ping Dave Malcolm to fix up his scripts.

Yes, yes, and yes.  (And, now gone.)


> > +/* Return true if STMT is any of the OpenACC types specifically.  */
> > +
> > +static inline bool
> > +is_gimple_omp_oacc_specifically (const_gimple stmt)
> 
> Why not is_gimple_oacc or gimple_oacc_p ?

The idea is to make it clear in the name that STMT must be an OMP one.
Now renamed to the shorter is_gimple_omp_oacc.


> > @@ -99,15 +103,16 @@ enum gimplify_omp_var_data
> >  
> >  enum omp_region_type
> >  {
> > -  ORT_WORKSHARE = 0,
> > -  ORT_SIMD = 1,
> > -  ORT_PARALLEL = 2,
> > -  ORT_COMBINED_PARALLEL = 3,
> > -  ORT_TASK = 4,
> > -  ORT_UNTIED_TASK = 5,
> > -  ORT_TEAMS = 8,
> > -  ORT_TARGET_DATA = 16,
> > -  ORT_TARGET = 32
> > +  /* An undefined region type.  */
> > +  ORT_INVALID = 0,
> > +
> > +  ORT_WORKSHARE,
> > +  ORT_SIMD,
> > +  ORT_PARALLEL,
> > +  ORT_COMBINED_PARALLEL,
> > +  ORT_TASK,
> > +  ORT_TEAMS,
> > +  ORT_TARGET
> >  };
> 
> If you want to shift from bitmasks in the enum
> to extra on the side bits (why?), then combined
> for parallel is another thing.

Right, but I've now dropped (reverted) this and further gimplification
changes.  Maybe this is material for the next stage 1, but maybe not
useful enough.


> [...]

> > +static inline tree
> > +lookup_reduction (const char *id, omp_context *ctx)
> 
> Can't you use oacc_ in the name of OpenACC specific functions?

> [...]

Review comments for OpenACC reduction code have been addressed by Cesar.
(Thanks!)


> >  	case OMP_CLAUSE_DEFAULT:
> > +	  gcc_assert (!is_gimple_omp_oacc_specifically (ctx->stmt));
> >  	  ctx->default_kind = OMP_CLAUSE_DEFAULT_KIND (c);
> >  	  break;
> 
> For clauses which are not parsed for OpenACC directives I find
> the gcc_assert (!is_gimple_omp_oacc_specifically (ctx->stmt));
> completely unnecessary.

Now all removed.  My thinking was that some of those clauses are
parsed/generated not only in the front ends, but also synthesized in
middle end processing, and I wanted to catch those.

> > @@ -1625,13 +1799,41 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
> >  	case OMP_CLAUSE_DIST_SCHEDULE:
> >  	case OMP_CLAUSE_DEPEND:
> >  	case OMP_CLAUSE__CILK_FOR_COUNT_:
> > +	  gcc_assert (!is_gimple_omp_oacc_specifically (ctx->stmt));
> > +	  /* FALLTHRU */
> > +	case OMP_CLAUSE_IF:
> 
> [...] if there are
> some spots you want to keep them in for now, consider gcc_checking_assert
> instead.

Now using this a few times.


> > --- gcc/tree-nested.c
> > +++ gcc/tree-nested.c
> > @@ -627,6 +627,8 @@ walk_gimple_omp_for (gimple for_stmt,
> >      		     walk_stmt_fn callback_stmt, walk_tree_fn callback_op,
> >      		     struct nesting_info *info)
> >  {
> > +  gcc_assert (!is_gimple_omp_oacc_specifically (for_stmt));
> > +
> 
> That surely can be reached and you can easily construct testcase, can't you?
> 
> > @@ -1323,6 +1325,10 @@ convert_nonlocal_reference_stmt (gimple_stmt_iterator *gsi, bool *handled_ops_p,
> >  	}
> >        break;
> >  
> > +    case GIMPLE_OACC_KERNELS:
> > +    case GIMPLE_OACC_PARALLEL:
> > +      gcc_unreachable ();
> > +
> 
> Ditto etc.

Same reasoning as for gimple_copy given above.  (And,
asserts/gcc_unreachable now all gone.)


Do you want me to repost the OpenACC Middle End changes patch, or would
you be OK with reviewing the code on gomp-4_0-branch, diffing against the
last trunk merge point, 0fcfaa33cbf333ac69cc2b01a7277e5272ff8a3d,
r218679?


GrÃÃe,
 Thomas

Attachment: pgptFcNUH51nM.pgp
Description: PGP signature