Hi Frederik! On 2019-12-10T15:23:01+0100, Frederik Harwath <frede...@codesourcery.com> wrote: > On 09.12.19 16:58, Harwath, Frederik wrote: >> Tobias has recently fixed a problem with the column information in gfortran >> locations >> [...] >> I have tested the patch manually by adapting the validity check for nested >> OpenACC reductions (see omp-low.c) >> to include the location of clauses in warnings instead of the location of >> the loop to which the clause belongs. >> I can add a regression test based on this later on after adapting the code >> in omp-low.c. > > here are patches adding the promised test for Fortran and a corresponding > test for C. > > Is it ok to include them in trunk?
Thanks, yes, with my following remarks considered, and acted on per your preference. To record the review effort, please include "Reviewed-by: Thomas Schwinge <tho...@codesourcery.com>" in the commit log, see <https://gcc.gnu.org/wiki/Reviewed-by>. > Frederik Harwath (2): > Use clause locations in OpenACC nested reduction warnings > Add tests to verify OpenACC clause locations I won't insist, but suggest (common practice) to merge that into one patch: bug fix plus test cases, using the summary line of your first patch. On 2019-12-10T15:23:02+0100, Frederik Harwath <frede...@codesourcery.com> wrote: > Since the Fortran front-end now sets the clause locations correctly, we can > emit warnings with more precise locations if we encounter conflicting > operations for a variable in reduction clauses. > > 2019-12-10 Frederik Harwath <frede...@codesourcery.com> > > gcc/ > * omp-low.c (scan_omp_for): Use clause location in warning. > --- a/gcc/omp-low.c > +++ b/gcc/omp-low.c > @@ -2473,7 +2473,7 @@ scan_omp_for (gomp_for *stmt, omp_context *outer_ctx) > tree_code outer_op = OMP_CLAUSE_REDUCTION_CODE (outer_clause); > if (outer_var == local_var && outer_op != local_op) > { > - warning_at (gimple_location (stmt), 0, > + warning_at (OMP_CLAUSE_LOCATION (local_clause), 0, > "conflicting reduction operations for %qE", > local_var); > inform (OMP_CLAUSE_LOCATION (outer_clause), Watch me think aloud: doesn't the same also apply to the other 'warning_at' added as part of "Warn about inconsistent OpenACC nested reduction clauses", the "nested loop in reduction needs [...]" diagnotic? Haha, of course it doesn't -- in that situation we don't have a specific clause location, because we don't have a 'reduction' clause. ;-) On 2019-12-10T15:23:03+0100, Frederik Harwath <frede...@codesourcery.com> wrote: > Check that the column information for OpenACC clauses is communicated > correctly > to the middle-end, in particular by the Fortran front-end (cf. PR 92793). > > 2019-12-10 Frederik Harwath <frede...@codesourcery.com> > > gcc/testsuite/ > * gcc.dg/goacc/clause-locations.c: New test. > * gfortran.dg/goacc/clause-locations.f90: New test. It's of course always OK to add new test cases, but wouldn't the same test coverage be reached by just adding such checking to the existing test cases in 'c-c++-common/goacc/nested-reductions-warn.c', 'gfortran.dg/goacc/nested-reductions-warn.f90'? That said: > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/goacc/clause-locations.c May want to into 'c-c++-common/', so that both C and C++ will be tested. > @@ -0,0 +1,17 @@ > +/* Verify that the location information for clauses is correct. */ > + > +void > +check_clause_columns() { > + int i, j, sum, diff; > + > + #pragma acc parallel > + { > + #pragma acc loop reduction(+:sum) > + for (i = 1; i <= 10; i++) > + { > + #pragma acc loop reduction(-:diff) reduction(-:sum) /* { dg-warning > "53: conflicting reduction operations for .sum." } */ > + for (j = 1; j <= 10; j++) > + sum = 1; > + } > + } > +} > --- /dev/null > +++ b/gcc/testsuite/gfortran.dg/goacc/clause-locations.f90 > @@ -0,0 +1,18 @@ > +! Verify that the location information for clauses is correct. > +! See also PR 92793. > + > +subroutine check_clause_columns () > + implicit none (type, external) > + integer :: i, j, sum, diff > + > + !$acc parallel > + !$acc loop reduction(+:sum) > + do i = 1, 10 > + !$acc loop reduction(-:diff) reduction(-:sum) ! { dg-warning "47: > conflicting reduction operations for .sum." } > + do j = 1, 10 > + sum = 1 > + end do > + end do > + !$acc end parallel > +end subroutine check_clause_columns > + Grüße Thomas
