[1/3] Correct the reported line number in fortran combined OpenACC directives

Message ID 9787e49d3bb2bc96b16584590090e33b2f4dcce8.1532531520.git.cesar@codesourcery.com
State New
Headers show
Series
  • Add OpenACC diagnostics to -fopt-info-note-omp
Related show

Commit Message

Cesar Philippidis July 25, 2018, 3:29 p.m.
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)

-- 
2.7.4

Comments

Marek Polacek July 25, 2018, 3:32 p.m. | #1
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
Cesar Philippidis July 25, 2018, 3:53 p.m. | #2
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 = &block;
   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);
 }
-- 
2.17.1
Thomas Schwinge Dec. 9, 2018, 1:07 p.m. | #3
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

Patch

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);
 }