Hi Jakub, On 06.11.19 14:00, Jakub Jelinek wrote: > On Wed, Nov 06, 2019 at 01:41:47PM +0100, frede...@codesourcery.com wrote: >> --- a/gcc/omp-low.c >> +++ b/gcc/omp-low.c >> @@ -128,6 +128,12 @@ struct omp_context >> [...] >> + /* A tree_list of the reduction clauses in this context. */ >> + tree local_reduction_clauses; >> + >> + /* A tree_list of the reduction clauses in outer contexts. */ >> + tree outer_reduction_clauses; > > Could there be acc in the name to make it clear it is OpenACC only?
Yes, will be added. >> @@ -910,6 +916,8 @@ new_omp_context (gimple *stmt, omp_context *outer_ctx) >> [...] >> + ctx->local_reduction_clauses = NULL; >> [...] >> @@ -925,6 +933,8 @@ new_omp_context (gimple *stmt, omp_context *outer_ctx) >> [...] >> + ctx->local_reduction_clauses = NULL; >> + ctx->outer_reduction_clauses = NULL; > > The = NULL assignments are unnecessary in all 3 cases, ctx is allocated with > XCNEW. Ok, will be removed. >> @@ -1139,6 +1149,11 @@ scan_sharing_clauses (tree clauses, omp_context *ctx) >> goto do_private; >> >> case OMP_CLAUSE_REDUCTION: >> + if (is_oacc_parallel (ctx) || is_oacc_kernels (ctx)) >> + ctx->local_reduction_clauses >> + = tree_cons (NULL, c, ctx->local_reduction_clauses); > > I'm not sure it is a good idea to use a TREE_LIST in this case, vec would be > more natural, wouldn't it. Yes. > Or, wouldn't it be better to do this checking in the gimplifier instead of > omp-low.c? There we have splay trees with GOVD_REDUCTION etc. for the > variables, so it wouldn't be O(#reductions^2) compile time> It is true that > the gimplifier doesn't record the reduction codes (after > all, OpenMP has UDRs and so there can be fairly arbitrary reductions). Right, I have considered moving the implementation somewhere else before. I am going to look into this, but perhaps we will just keep it where it is if otherwise the implementation becomes more complicated. > Consider million reduction clauses on nested loops. > If gimplifier is not the right spot, then use a splay tree + vector instead? > splay tree for the outer ones, vector for the local ones, and put into both > the clauses, so you can compare reduction code etc. Sounds like a good idea. I am going to try that. However, I have not seen the suboptimal data structure choices of the original patch as a problem, since the case of million reduction clauses has not occurred to me. Thank you for your feedback! Best regards, Frederik
