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 <ja...@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
pgpt9HC17Kacl.pgp
Description: PGP signature
