*ping* - Re: [Patch] Rework OpenACC nested reduction clause consistency checking (was: Re: [PATCH][committed] Warn about inconsistent OpenACC nested reduction clauses)
PING Hi Jakub, I have attached a version of the patch that has been rebased on the current trunk. Frederik On 03.12.19 12:16, Harwath, Frederik wrote: > On 08.11.19 07:41, Harwath, Frederik wrote: >> On 06.11.19 14:00, Jakub Jelinek wrote: >> [...] >>> 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. >> [...] >>> 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. > > Below you can find a patch that reimplements the nested reductions check using > more appropriate data structures. [...] From b08855328c52e36143770e442e50ba87f25c14b3 Mon Sep 17 00:00:00 2001 From: Frederik Harwath Date: Wed, 8 Jan 2020 14:00:44 +0100 Subject: [PATCH] Rework OpenACC nested reduction clause consistency checking Revision 277875 of trunk introduced a consistency check for nested OpenACC reduction clauses. The implementation has two drawbacks: 1) It uses suboptimal data structures for storing information about the reduction clauses. 2) The warnings issued for *repeated* inconsistent use of reduction operators are confusing. For instance, on three nested loops that use the reduction operators +, -, + on the same variable, we obtain a warning at the switch from + to - (as desired) and another warning about the switch from - to +. It would be preferable to avoid the second warning since + is consistent with the first reduction operator. This commit attempts to fix both problems by using more appropriate data structures (splay trees and vectors instead of tree lists) for keeping track of the information about the reduction clauses. 2020-01-08 Frederik Harwath gcc/ * omp-low.c (omp_context): Removed fields local_reduction_clauses, outer_reduction_clauses; added fields oacc_reduction_clauses, oacc_reductions_stack. (oacc_reduction_clause_location): New struct. (oacc_reduction_var_occ): New struct. (new_omp_context): Adjust omp_context initialization to new fields. (delete_omp_context): Adjust omp_context deletion to new fields. (rewind_oacc_reductions_stack): New function. (check_oacc_reduction_clause): New function. (check_oacc_reduction_clauses): New function. (scan_sharing_clauses): Call check_oacc_reduction_clause for reduction clauses (this handles clauses on compute regions) if a new optional flag is enabled. (scan_omp_for): Remove old nested reduction check, call check_oacc_reduction_clauses instead. (scan_omp_target): Adapt call to scan_sharing_clauses to enable the new flag. gcc/testsuite/ * c-c++-common/goacc/nested-reductions-warn.c: Add dg-prune-output to ignore warnings that are not relevant to the test. (acc_parallel): Stop expecting pruned warnings, adjust expected warnings to changes in omp-low.c, add checks for info messages about the location of clauses. (acc_parallel_loop): Likewise. (acc_parallel_reduction): Likewise. (acc_parallel_loop_reduction): Likewise. (acc_routine): Likewise. (acc_kernels): Likewise. * gfortran.dg/goacc/nested-reductions-warn.f90: Likewise. --- gcc/omp-low.c | 306 -- .../goacc/nested-reductions-warn.c| 81 ++--- .../goacc/nested-reductions-warn.f90 | 83 ++--- 3 files changed, 271 insertions(+), 199 deletions(-) diff --git a/gcc/omp-low.c b/gcc/omp-low.c index e692a53a3de..6026b7aff89 100644 --- a/gcc/omp-low.c +++ b/gcc/omp-low.c @@ -73,6 +73,9 @@ along with GCC; see the file COPYING3. If not see scanned for regions which are then moved to a new function, to be invoked by the thread library, or offloaded. */ + +struct oacc_reduction_var_occ; + /* Context structure. Used to store information about each parallel directive in the code. */ @@ -128,12 +131,6 @@ struct omp_context corresponding tracking loop iteration variables. */ hash_map *lastprivate_conditional_map; - /* 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; - /* Nesting depth of this context. Used to beautify error messages re invalid gotos. The outermost ctx is depth 1, with depth 0 being reserved for the main body of the function. */ @@ -163,8 +160,52 @@ struct omp_context /* True if there is bind clause on the construct (i.e. a loop construct). */ bool loop_p; + + /* A mapping that maps a variable to information about the last OpenACC + reduction clause that used the variable above the current context. + This information is used for checking the nesting restrictions for + reduction clauses by the function check_oacc_reduction_clauses. + The mapping is owned by the outermost context (i.e. a context
[Patch] Rework OpenACC nested reduction clause consistency checking (was: Re: [PATCH][committed] Warn about inconsistent OpenACC nested reduction clauses)
Hi Jakub, On 08.11.19 07:41, Harwath, Frederik wrote: > On 06.11.19 14:00, Jakub Jelinek wrote: > [...] >> 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. > > [...] >> 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. Below you can find a patch that reimplements the nested reductions check using more appropriate data structures. As an additional benefit, the quality of the warnings has also improved (see description below). I have checked the patch by running the testsuite on x86_64-pc-linux-gnu. Best regards, Frederik From 94ca786172afa7dab7630d75965bf6d6f0dd24e1 Mon Sep 17 00:00:00 2001 From: Frederik Harwath Date: Tue, 3 Dec 2019 10:38:01 +0100 Subject: [PATCH] Rework OpenACC nested reduction clause consistency checking Revision 277875 of trunk introduced a consistency check for nested OpenACC reduction clauses. The implementation has two drawbacks: 1) It uses suboptimal data structures for storing information about the reduction clauses. 2) The warnings issued for *repeated* inconsistent use of reduction operators are confusing. For instance, on three nested loops that use the reduction operators +, -, + on the same variable, we obtain a warning at the switch from + to - (as desired) and another warning about the switch from - to +. It would be preferable to avoid the second warning since + is consistent with the first reduction operator. This commit attempts to fix both problems by using more appropriate data structures (splay trees and vectors instead of tree lists) for keeping track of the information about the reduction clauses. 2019-12-3 Frederik Harwath gcc/ * omp-low.c (omp_context): Removed fields local_reduction_clauses, outer_reduction_clauses; added fields oacc_reduction_clauses, oacc_reductions_stack. (oacc_reduction_clause_location): New struct. (oacc_reduction_var_occ): New struct. (new_omp_context): Adjust omp_context initialization to new fields. (delete_omp_context): Adjust omp_context deletion to new fields. (rewind_oacc_reductions_stack): New function. (check_oacc_reduction_clause): New function. (check_oacc_reduction_clauses): New function. (scan_sharing_clauses): Call check_oacc_reduction_clause for reduction clauses (this handles clauses on compute regions) if a new optional flag is enabled. (scan_omp_for): Remove old nested reduction check, call check_oacc_reduction_clauses instead. (scan_omp_target): Adapt call to scan_sharing_clauses to enable the new flag. gcc/testsuite/ * c-c++-common/goacc/nested-reductions-warn.c: Add dg-prune-output to ignore warnings that are not relevant to the test. (acc_parallel): Stop expecting pruned warnings, adjust expected warnings to changes in omp-low.c, add checks for info messages about the location of clauses. (acc_parallel_loop): Likewise. (acc_parallel_reduction): Likewise. (acc_parallel_loop_reduction): Likewise. (acc_routine): Likewise. (acc_kernels): Likewise. * gfortran.dg/goacc/nested-reductions-warn.f90: Likewise. --- gcc/omp-low.c | 305 -- .../goacc/nested-reductions-warn.c| 81 ++--- .../goacc/nested-reductions-warn.f90 | 83 ++--- 3 files changed, 271 insertions(+), 198 deletions(-) diff --git a/gcc/omp-low.c b/gcc/omp-low.c index 19132f76da2..ba04e7477dc 100644 --- a/gcc/omp-low.c +++ b/gcc/omp-low.c @@ -73,6 +73,9 @@ along with GCC; see the file COPYING3. If not see scanned for regions which are then moved to a new function, to be invoked by the thread library, or offloaded. */ + +struct oacc_reduction_var_occ; + /* Context structure. Used to store information about each parallel directive in the code. */ @@ -128,12 +131,6 @@ struct omp_context corresponding tracking loop iteration variables. */ hash_map *lastprivate_conditional_map; - /* 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; - /* Nesting depth of this context. Used to beautify error messages re invalid gotos. The outermost ctx is depth 1, with depth 0 being reserved for the main body of the function. */ @@ -163,8 +160,52 @@ struct omp_context /* True if there is bind clause on the construct (i.e. a loop construct). */ bool loop_p; + + /* A mapping that maps a variable to information about the last OpenACC + reduction clause that used the variable above the current context. + This information is used for checking the nesting restrictions for + reduction clauses by the function check_oacc_reduction_clauses. + The mapping is owned by
