| Message ID | 9787e49d3bb2bc96b16584590090e33b2f4dcce8.1532531520.git.cesar@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add OpenACC diagnostics to -fopt-info-note-omp | expand |
On Wed, Jul 25, 2018 at 08:29:17AM -0700, Cesar Philippidis wrote: > The fortran FE incorrectly records the line locations of combined acc > loop directives when it lowers the construct to gimple. Usually this > isn't a problem because the fortran FE is able to report problems with > acc loops itself. However, there will be inaccuracies if the ME tries > to use those locations. > > Note that test cases are inconspicuously absent in this patch. > However, without this bug fix, -fopt-info-note-omp will report bogus > line numbers. This code patch will be tested in a later patch in > this series. > > Is this OK for trunk? I bootstrapped and regtested it on x86_64 with > nvptx offloading. > > Thanks, > Cesar > > 2018-XX-YY Cesar Philippidis <cesar@codesourcery.com> > > gcc/fortran/ > * trans-openmp.c (gfc_trans_oacc_combined_directive): Set the > location of combined acc loops. > > (cherry picked from gomp-4_0-branch r245653) > > diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c > index f038f4c..e7707d0 100644 > --- a/gcc/fortran/trans-openmp.c > +++ b/gcc/fortran/trans-openmp.c > @@ -3869,6 +3869,7 @@ gfc_trans_oacc_combined_directive (gfc_code *code) > gfc_omp_clauses construct_clauses, loop_clauses; > tree stmt, oacc_clauses = NULL_TREE; > enum tree_code construct_code; > + location_t loc = input_location; > > switch (code->op) > { > @@ -3930,12 +3931,16 @@ gfc_trans_oacc_combined_directive (gfc_code *code) > else > pushlevel (); > stmt = gfc_trans_omp_do (code, EXEC_OACC_LOOP, pblock, &loop_clauses, NULL); > + > + if (CAN_HAVE_LOCATION_P (stmt)) > + SET_EXPR_LOCATION (stmt, loc); This is protected_set_expr_location. Marek
On 07/25/2018 08:32 AM, Marek Polacek wrote: > On Wed, Jul 25, 2018 at 08:29:17AM -0700, Cesar Philippidis wrote: >> The fortran FE incorrectly records the line locations of combined acc >> loop directives when it lowers the construct to gimple. Usually this >> isn't a problem because the fortran FE is able to report problems with >> acc loops itself. However, there will be inaccuracies if the ME tries >> to use those locations. >> >> Note that test cases are inconspicuously absent in this patch. >> However, without this bug fix, -fopt-info-note-omp will report bogus >> line numbers. This code patch will be tested in a later patch in >> this series. >> >> Is this OK for trunk? I bootstrapped and regtested it on x86_64 with >> nvptx offloading. >> >> Thanks, >> Cesar >> >> 2018-XX-YY Cesar Philippidis <cesar@codesourcery.com> >> >> gcc/fortran/ >> * trans-openmp.c (gfc_trans_oacc_combined_directive): Set the >> location of combined acc loops. >> >> (cherry picked from gomp-4_0-branch r245653) >> >> diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c >> index f038f4c..e7707d0 100644 >> --- a/gcc/fortran/trans-openmp.c >> +++ b/gcc/fortran/trans-openmp.c >> @@ -3869,6 +3869,7 @@ gfc_trans_oacc_combined_directive (gfc_code *code) >> gfc_omp_clauses construct_clauses, loop_clauses; >> tree stmt, oacc_clauses = NULL_TREE; >> enum tree_code construct_code; >> + location_t loc = input_location; >> >> switch (code->op) >> { >> @@ -3930,12 +3931,16 @@ gfc_trans_oacc_combined_directive (gfc_code *code) >> else >> pushlevel (); >> stmt = gfc_trans_omp_do (code, EXEC_OACC_LOOP, pblock, &loop_clauses, NULL); >> + >> + if (CAN_HAVE_LOCATION_P (stmt)) >> + SET_EXPR_LOCATION (stmt, loc); > > This is protected_set_expr_location. Neat, thanks! This patch includes that correction. Is it ok for trunk after bootstrapping and regression testing? Thanks, Cesar 2018-XX-YY Cesar Philippidis <cesar@codesourcery.com> gcc/fortran/ * trans-openmp.c (gfc_trans_oacc_combined_directive): Set the location of combined acc loops. (cherry picked from gomp-4_0-branch r245653) --- gcc/fortran/trans-openmp.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c index f038f4c5bf8..b549c682533 100644 --- a/gcc/fortran/trans-openmp.c +++ b/gcc/fortran/trans-openmp.c @@ -3869,6 +3869,7 @@ gfc_trans_oacc_combined_directive (gfc_code *code) gfc_omp_clauses construct_clauses, loop_clauses; tree stmt, oacc_clauses = NULL_TREE; enum tree_code construct_code; + location_t loc = input_location; switch (code->op) { @@ -3929,13 +3930,16 @@ gfc_trans_oacc_combined_directive (gfc_code *code) pblock = █ else pushlevel (); + stmt = gfc_trans_omp_do (code, EXEC_OACC_LOOP, pblock, &loop_clauses, NULL); + protected_set_expr_location (stmt, loc); + if (TREE_CODE (stmt) != BIND_EXPR) stmt = build3_v (BIND_EXPR, NULL, stmt, poplevel (1, 0)); else poplevel (0, 0); - stmt = build2_loc (input_location, construct_code, void_type_node, stmt, - oacc_clauses); + + stmt = build2_loc (loc, construct_code, void_type_node, stmt, oacc_clauses); gfc_add_expr_to_block (&block, stmt); return gfc_finish_block (&block); }
Hi! On Wed, 25 Jul 2018 08:53:35 -0700, Cesar Philippidis <cesar@codesourcery.com> wrote: > On 07/25/2018 08:32 AM, Marek Polacek wrote: > > On Wed, Jul 25, 2018 at 08:29:17AM -0700, Cesar Philippidis wrote: > >> The fortran FE incorrectly records the line locations of combined acc > >> loop directives when it lowers the construct to gimple. After a bit of preparational work to "use existing middle end checking for Fortran OpenACC loop clauses"... > >> Usually this > >> isn't a problem because the fortran FE is able to report problems with > >> acc loops itself. ..., the Fortran front end is no longer doing that, and... > >> However, there will be inaccuracies if the ME tries > >> to use those locations. > >> > >> Note that test cases are inconspicuously absent in this patch. ..., I've been able to verify your changes by translating your C++ test case into Fortran. > >> However, without this bug fix, -fopt-info-note-omp will report bogus > >> line numbers. This code patch will be tested in a later patch in > >> this series. > >> + if (CAN_HAVE_LOCATION_P (stmt)) > >> + SET_EXPR_LOCATION (stmt, loc); > > > > This is protected_set_expr_location. > > Neat, thanks! This patch includes that correction. Is it ok for trunk > after bootstrapping and regression testing? Thanks, committed to trunk in r266924: commit a43ff24656fa8224b249e159ea81e629ffa32664 Author: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4> Date: Sun Dec 9 12:49:20 2018 +0000 Correct the reported line number in Fortran combined OpenACC directives gcc/fortran/ * trans-openmp.c (gfc_trans_oacc_combined_directive): Set the location of combined acc loops. gcc/testsuite/ * gfortran.dg/goacc/combined-directives-3.f90: New file. Reviewed-by: Thomas Schwinge <thomas@codesourcery.com> git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@266924 138bc75d-0d04-0410-961f-82ee72b054a4 --- gcc/fortran/ChangeLog | 5 +++++ gcc/fortran/trans-openmp.c | 5 +++-- gcc/testsuite/ChangeLog | 4 ++++ .../c-c++-common/goacc/combined-directives-3.c | 1 + .../gfortran.dg/goacc/combined-directives-3.f90 | 26 ++++++++++++++++++++++ 5 files changed, 39 insertions(+), 2 deletions(-) diff --git gcc/fortran/ChangeLog gcc/fortran/ChangeLog index c6eb05174f69..e74bda7a1362 100644 --- gcc/fortran/ChangeLog +++ gcc/fortran/ChangeLog @@ -1,3 +1,8 @@ +2018-12-09 Cesar Philippidis <cesar@codesourcery.com> + + * trans-openmp.c (gfc_trans_oacc_combined_directive): Set the + location of combined acc loops. + 2018-12-09 Thomas Schwinge <thomas@codesourcery.com> * openmp.c (resolve_oacc_loop_blocks): Remove checking of OpenACC diff --git gcc/fortran/trans-openmp.c gcc/fortran/trans-openmp.c index c9fc4e49c450..bf3f46939e39 100644 --- gcc/fortran/trans-openmp.c +++ gcc/fortran/trans-openmp.c @@ -3878,6 +3878,7 @@ gfc_trans_oacc_combined_directive (gfc_code *code) gfc_omp_clauses construct_clauses, loop_clauses; tree stmt, oacc_clauses = NULL_TREE; enum tree_code construct_code; + location_t loc = input_location; switch (code->op) { @@ -3939,12 +3940,12 @@ gfc_trans_oacc_combined_directive (gfc_code *code) else pushlevel (); stmt = gfc_trans_omp_do (code, EXEC_OACC_LOOP, pblock, &loop_clauses, NULL); + protected_set_expr_location (stmt, loc); if (TREE_CODE (stmt) != BIND_EXPR) stmt = build3_v (BIND_EXPR, NULL, stmt, poplevel (1, 0)); else poplevel (0, 0); - stmt = build2_loc (input_location, construct_code, void_type_node, stmt, - oacc_clauses); + stmt = build2_loc (loc, construct_code, void_type_node, stmt, oacc_clauses); gfc_add_expr_to_block (&block, stmt); return gfc_finish_block (&block); } diff --git gcc/testsuite/ChangeLog gcc/testsuite/ChangeLog index 6b26f6f510db..19bc532c9d57 100644 --- gcc/testsuite/ChangeLog +++ gcc/testsuite/ChangeLog @@ -1,3 +1,7 @@ +2018-12-09 Thomas Schwinge <thomas@codesourcery.com> + + * gfortran.dg/goacc/combined-directives-3.f90: New file. + 2018-12-09 Cesar Philippidis <cesar@codesourcery.com> * c-c++-common/goacc/combined-directives-3.c: New test. diff --git gcc/testsuite/c-c++-common/goacc/combined-directives-3.c gcc/testsuite/c-c++-common/goacc/combined-directives-3.c index 77d418262eac..c6e31c26a8f1 100644 --- gcc/testsuite/c-c++-common/goacc/combined-directives-3.c +++ gcc/testsuite/c-c++-common/goacc/combined-directives-3.c @@ -1,5 +1,6 @@ /* Verify the accuracy of the line number associated with combined constructs. */ +/* See also "../../gfortran.dg/goacc/combined-directives-3.f90". */ int main () diff --git gcc/testsuite/gfortran.dg/goacc/combined-directives-3.f90 gcc/testsuite/gfortran.dg/goacc/combined-directives-3.f90 new file mode 100644 index 000000000000..b138822827f6 --- /dev/null +++ gcc/testsuite/gfortran.dg/goacc/combined-directives-3.f90 @@ -0,0 +1,26 @@ +! Verify the accuracy of the line number associated with combined constructs. +! See "../../c-c++-common/goacc/combined-directives-3.c". + +subroutine test + implicit none + integer x, y, z + + !$acc parallel loop seq auto ! { dg-error "'seq' overrides other OpenACC loop specifiers" } + do x = 0, 10 + !$acc loop + do y = 0, 10 + end do + end do + !$acc end parallel loop + + !$acc parallel loop gang auto ! { dg-error "'auto' conflicts with other OpenACC loop specifiers" } + do x = 0, 10 + !$acc loop worker auto ! { dg-error "'auto' conflicts with other OpenACC loop specifiers" } + do y = 0, 10 + !$acc loop vector + do z = 0, 10 + end do + end do + end do + !$acc end parallel loop +end subroutine test Grüße Thomas
diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c index f038f4c..e7707d0 100644 --- a/gcc/fortran/trans-openmp.c +++ b/gcc/fortran/trans-openmp.c @@ -3869,6 +3869,7 @@ gfc_trans_oacc_combined_directive (gfc_code *code) gfc_omp_clauses construct_clauses, loop_clauses; tree stmt, oacc_clauses = NULL_TREE; enum tree_code construct_code; + location_t loc = input_location; switch (code->op) { @@ -3930,12 +3931,16 @@ gfc_trans_oacc_combined_directive (gfc_code *code) else pushlevel (); stmt = gfc_trans_omp_do (code, EXEC_OACC_LOOP, pblock, &loop_clauses, NULL); + + if (CAN_HAVE_LOCATION_P (stmt)) + SET_EXPR_LOCATION (stmt, loc); + if (TREE_CODE (stmt) != BIND_EXPR) stmt = build3_v (BIND_EXPR, NULL, stmt, poplevel (1, 0)); else poplevel (0, 0); - stmt = build2_loc (input_location, construct_code, void_type_node, stmt, - oacc_clauses); + + stmt = build2_loc (loc, construct_code, void_type_node, stmt, oacc_clauses); gfc_add_expr_to_block (&block, stmt); return gfc_finish_block (&block); }