This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][committed] Warn about inconsistent OpenACC nested reduction clauses
- From: "Harwath, Frederik" <frederik at codesourcery dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: <gcc-patches at gcc dot gnu dot org>, <thomas at codesourcery dot com>
- Date: Fri, 8 Nov 2019 07:41:22 +0100
- Subject: Re: [PATCH][committed] Warn about inconsistent OpenACC nested reduction clauses
- Ironport-sdr: 2DYkOK5cf+ezPJR5nYOZf0k7AeQ7FVHR0Tejdddn1b4PxDuCk6QfkjZLN8s/jWtA9EpCR7bSF+ AvVuG7ixl7ajHqve0PkvmLVIPDNEJlK2zAXT8HW+miM4/1B2ebLp/7mNNLhj5UoF8443Og+40q qEX0XWsP4GCI5a+VmzCdvOzEeXJMvzlhkWJ4zkcwU0fiqUPQmVUARyBs0EPq2P2unisMJuRUQ5 O+ejIttRYETvYRn9Lncjn1JViIP2GHWxeimqUch9Jt37tFX5gbRmkhmFj6haS5WtGPWHsGYKg6 TzM=
- Ironport-sdr: a47SQ205DBIIf5KBxkt7s78zLWvhQJ32LtXdVvjNggxWy55kh8OkqG7ZBQpUcutVQa/pZEfC+u 2BW3apr8/gugGFVmpkLcw8djmg8pPktDxcJaxiFJdBmdgyPKZ1DHngNUfLC1FQkVoOXKsQfxiC UEJ1aD0rcxbYhXzjO4vo58F7VQU+OABRdoB8ppsEJmHkaV7eQ8acy3Ttsog235SUsXvBryCIs3 5xt9XY/CQYb3u8eUMUlkgH1CaDdgVhi+naAAcIBWbHMjAaITUKV4wIHP2Xy9nW1lZnRkyX01uE JMc=
- References: <87y2wuqu6h.fsf@euler.schwinge.homeip.net> <20191106124148.26041-1-frederik@codesourcery.com> <20191106130037.GB4650@tucnak>
Hi Jakub,
On 06.11.19 14:00, Jakub Jelinek wrote:
> On Wed, Nov 06, 2019 at 01:41:47PM +0100, frederik@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