Re: [PATCH 0/2] Add tests to verify OpenACC clause locations

[Thomas Schwinge]

Hi!

On 2020-11-03T09:17:49+0100, I wrote:
> I've just pushed "[OpenACC] More precise diagnostics for 'gang',
> 'worker', 'vector' clauses with arguments on 'loop' only allowed in
> 'kernels' regions" to master branch in commit
> beddd1762ad2bbe84dd776c54489153f83f21e56, and backported to
> releases/gcc-10 in commit 8d09f49006ce4c2f8d4018206c12e131c49ca6ce

> [...], and 'inform' at the location of the enclosing parent
> compute construct/[...].

Now really.  I've pushed "[OpenACC] Use proper location to 'inform' of
enclosing parent compute construct" to master branch in commit
fab72592d86d11b89a01f0f3c2c9c329d43466c1, and backported to
releases/gcc-10 branch in commit
a32d089dcf3edaa625e4871e78150b7d297cda5b, see attached.


Grüße
 Thomas


-
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander 
Walter
>From fab72592d86d11b89a01f0f3c2c9c329d43466c1 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Tue, 3 Nov 2020 22:06:29 +0100
Subject: [PATCH] [OpenACC] Use proper location to 'inform' of enclosing parent
 compute construct

Bug fix for recent commit beddd1762ad2bbe84dd776c54489153f83f21e56 "[OpenACC]
More precise diagnostics for 'gang', 'worker', 'vector' clauses with arguments
on 'loop' only allowed in 'kernels' regions":

> [...], and 'inform' at the location of the enclosing parent
> compute construct/[...].

Now really.

	gcc/
	* omp-low.c (scan_omp_for) : Use proper location to
	'inform' of enclosing parent compute construct.
	gcc/testsuite/
	* c-c++-common/goacc/pr92793-1.c: Extend.
	* gfortran.dg/goacc/pr92793-1.f90: Likewise.
---
 gcc/omp-low.c |  2 +-
 gcc/testsuite/c-c++-common/goacc/pr92793-1.c  | 58 +--
 gcc/testsuite/gfortran.dg/goacc/pr92793-1.f90 | 55 --
 3 files changed, 104 insertions(+), 11 deletions(-)

diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index 2f1a544bd46e..ea9008b61c41 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -2443,7 +2443,7 @@ scan_omp_for (gomp_for *stmt, omp_context *outer_ctx)
 			  "argument not permitted on %qs clause",
 			  omp_clause_code_name[OMP_CLAUSE_CODE (c)]);
 		if (tgt)
-		  inform (gimple_location (outer_ctx->stmt),
+		  inform (gimple_location (tgt->stmt),
 			  "enclosing parent compute construct");
 		else if (oacc_get_fn_attrib (current_function_decl))
 		  inform (DECL_SOURCE_LOCATION (current_function_decl),
diff --git a/gcc/testsuite/c-c++-common/goacc/pr92793-1.c b/gcc/testsuite/c-c++-common/goacc/pr92793-1.c
index 77ebb20265cf..71a556e27513 100644
--- a/gcc/testsuite/c-c++-common/goacc/pr92793-1.c
+++ b/gcc/testsuite/c-c++-common/goacc/pr92793-1.c
@@ -57,7 +57,7 @@ reduction(-:sum  ) /* { dg-line sum2 } */ \
 
 
 void
-a_sl() {
+gwv_sl_1() {
 #pragma acc serial loop /* { dg-message "9: enclosing parent compute construct" } */ \
 gang(num:5) /* { dg-error "5: argument not permitted on 'gang' clause" } */ \
   worker(num:5) /* { dg-error "3: argument not permitted on 'worker' clause" } */ \
@@ -67,7 +67,25 @@ a_sl() {
 }
 
 void
-a_s_l() {
+gwv_sl_2() {
+#pragma acc serial loop /* { dg-message "9: enclosing parent compute construct" } */
+  for (int i = 0; i < 10; i++)
+{
+#pragma acc loop /* { dg-bogus "enclosing parent compute construct" } */
+  for (int j = 0; j < 10; j++)
+	{
+#pragma acc loop \
+gang(num:5) /* { dg-error "9: argument not permitted on 'gang' clause" } */ \
+worker(num:5) /* { dg-error "5: argument not permitted on 'worker' clause" } */ \
+  vector(length:5) /* { dg-error "3: argument not permitted on 'vector' clause" } */
+	  for (int k = 0; k < 10; k++)
+	;
+	}
+}
+}
+
+void
+gwv_s_l() {
 #pragma acc serial /* { dg-message "9: enclosing parent compute construct" } */
   {
 #pragma acc loop \
@@ -76,18 +94,48 @@ a_s_l() {
   vector(length:5) /* { dg-error "3: argument not permitted on 'vector' clause" } */
 for (int i = 0; i < 10; i++)
   ;
+
+#pragma acc loop
+for (int i = 0; i < 10; i++)
+  {
+#pragma acc loop /* { dg-bogus "enclosing parent compute construct" } */
+	for (int j = 0; j < 10; j++)
+	  {
+#pragma acc loop \
+ gang(num:5) /* { dg-error "10: argument not permitted on 'gang' clause" } */ \
+  worker(num:5) /* { dg-error "7: argument not permitted on 'worker' clause" } */ \
+vector(length:5) /* { dg-error "5: argument not permitted on 'vector' clause" } */
+	for (int k = 0; k < 10; k++)
+	  ;
+	  }
+  }
   }
 }
 
-void a_r();
-#pragma acc routine(a_r)
+void gwv_r();
+#pragma acc routine(gwv_r)
 
 void
-a_r() { /* { dg-message "1: enclosing routine" } */
+gwv_r() { /* { dg-message "1: enclosing routine" } */
 #pragma acc loop \
gang(num:5) /* { dg-error "4: argument not permitted on 'gang' clause" } */ \
 worker(num:5) /* { dg-error "5: argument not permitted on 'worker' clause" } */ \
   vector(length:5) /* { dg-error "3: argument not permitted 

Re: [PATCH 0/2] Add tests to verify OpenACC clause locations

[Thomas Schwinge]

Hi!

On 2019-12-10T15:23:01+0100, Frederik Harwath  wrote:
> On 09.12.19 16:58, Harwath, Frederik wrote:
>> [use] the location of clauses in warnings instead of the location of the 
>> loop to which the clause belongs.

> Frederik Harwath (2):
>   Use clause locations in OpenACC nested reduction warnings

Basically:

-warning_at (gimple_location (stmt), 0,
+warning_at (OMP_CLAUSE_LOCATION (clause), 0,

>   Add tests to verify OpenACC clause locations

Similar changes are desirable for other directives/clauses, too.

I've just pushed "[OpenACC] More precise diagnostics for 'gang',
'worker', 'vector' clauses with arguments on 'loop' only allowed in
'kernels' regions" to master branch in commit
beddd1762ad2bbe84dd776c54489153f83f21e56, and backported to
releases/gcc-10 in commit 8d09f49006ce4c2f8d4018206c12e131c49ca6ce, see
attached.


Grüße
 Thomas


-
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander 
Walter
>From beddd1762ad2bbe84dd776c54489153f83f21e56 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Tue, 27 Oct 2020 17:13:16 +0100
Subject: [PATCH] [OpenACC] More precise diagnostics for 'gang', 'worker',
 'vector' clauses with arguments on 'loop' only allowed in 'kernels' regions

Instead of at the location of the 'loop' directive, 'error_at' the location of
the improper clause, and 'inform' at the location of the enclosing parent
compute construct/routine.

The Fortran testcases come with some XFAILing, to be resolved later.

	gcc/
	* omp-low.c (scan_omp_for) : More precise diagnostics for
	'gang', 'worker', 'vector' clauses with arguments only allowed in
	'kernels' regions.
	gcc/testsuite/
	* c-c++-common/goacc/pr92793-1.c: Extend.
	* gfortran.dg/goacc/pr92793-1.f90: Likewise.
---
 gcc/omp-low.c | 29 +++
 gcc/testsuite/c-c++-common/goacc/pr92793-1.c  | 37 +
 gcc/testsuite/gfortran.dg/goacc/pr92793-1.f90 | 52 +++
 3 files changed, 108 insertions(+), 10 deletions(-)

diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index 5392fa7e3086..de5142f979b0 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -2418,30 +2418,39 @@ scan_omp_for (gomp_for *stmt, omp_context *outer_ctx)
   if (!tgt || is_oacc_parallel_or_serial (tgt))
 	for (tree c = clauses; c; c = OMP_CLAUSE_CHAIN (c))
 	  {
-	char const *check = NULL;
-
+	tree c_op0;
 	switch (OMP_CLAUSE_CODE (c))
 	  {
 	  case OMP_CLAUSE_GANG:
-		check = "gang";
+		c_op0 = OMP_CLAUSE_GANG_EXPR (c);
 		break;
 
 	  case OMP_CLAUSE_WORKER:
-		check = "worker";
+		c_op0 = OMP_CLAUSE_WORKER_EXPR (c);
 		break;
 
 	  case OMP_CLAUSE_VECTOR:
-		check = "vector";
+		c_op0 = OMP_CLAUSE_VECTOR_EXPR (c);
 		break;
 
 	  default:
-		break;
+		continue;
 	  }
 
-	if (check && OMP_CLAUSE_OPERAND (c, 0))
-	  error_at (gimple_location (stmt),
-			"argument not permitted on %qs clause in"
-			" OpenACC % or %", check);
+	if (c_op0)
+	  {
+		error_at (OMP_CLAUSE_LOCATION (c),
+			  "argument not permitted on %qs clause",
+			  omp_clause_code_name[OMP_CLAUSE_CODE (c)]);
+		if (tgt)
+		  inform (gimple_location (outer_ctx->stmt),
+			  "enclosing parent compute construct");
+		else if (oacc_get_fn_attrib (current_function_decl))
+		  inform (DECL_SOURCE_LOCATION (current_function_decl),
+			  "enclosing routine");
+		else
+		  gcc_unreachable ();
+	  }
 	  }
 
   if (tgt && is_oacc_kernels (tgt))
diff --git a/gcc/testsuite/c-c++-common/goacc/pr92793-1.c b/gcc/testsuite/c-c++-common/goacc/pr92793-1.c
index d7a2ae487992..77ebb20265cf 100644
--- a/gcc/testsuite/c-c++-common/goacc/pr92793-1.c
+++ b/gcc/testsuite/c-c++-common/goacc/pr92793-1.c
@@ -54,3 +54,40 @@ reduction(-:sum  ) /* { dg-line sum2 } */ \
   }
   }
 }
+
+
+void
+a_sl() {
+#pragma acc serial loop /* { dg-message "9: enclosing parent compute construct" } */ \
+gang(num:5) /* { dg-error "5: argument not permitted on 'gang' clause" } */ \
+  worker(num:5) /* { dg-error "3: argument not permitted on 'worker' clause" } */ \
+   vector(length:5) /* { dg-error "4: argument not permitted on 'vector' clause" } */
+  for (int i = 0; i < 10; i++)
+;
+}
+
+void
+a_s_l() {
+#pragma acc serial /* { dg-message "9: enclosing parent compute construct" } */
+  {
+#pragma acc loop \
+   gang(num:5) /* { dg-error "8: argument not permitted on 'gang' clause" } */ \
+   worker(num:5) /* { dg-error "4: argument not permitted on 'worker' clause" } */ \
+  vector(length:5) /* { dg-error "3: argument not permitted on 'vector' clause" } */
+for (int i = 0; i < 10; i++)
+  ;
+  }
+}
+
+void a_r();
+#pragma acc routine(a_r)
+
+void
+a_r() { /* { dg-message "1: enclosing routine" } */
+#pragma acc loop \
+   gang(num:5) /* { dg-error "4: argument not permitted on 'gang' clause" } */ \
+worker(num:5) /* { dg-error "5: argument not permitted on 'worker' clause" } */ \
+ 

Re: [PATCH 0/2] Add tests to verify OpenACC clause locations

[Harwath, Frederik]

Hi Thomas,

On 10.12.19 15:44, Thomas Schwinge wrote:

> Thanks, yes, with my following remarks considered, and acted on per your
> preference.  To record the review effort, please include "Reviewed-by:
> Thomas Schwinge " in the commit log, see
> .

Committed as r279168 and r279169.

Frederik

Re: [PATCH 0/2] Add tests to verify OpenACC clause locations

[Harwath, Frederik]

Hi Thomas,

On 10.12.19 15:44, Thomas Schwinge wrote:

>> 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.> [...]
> 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'?

Sure, we could have everything in one patch and one test. The rationale
for splitting the patches and for splitting the tests is that the tests do
not try to verify the nested reductions validation code. They try to verify
that the language front-ends set the correct locations for clauses.
Without a possibility to do proper unit testing, I just had to find some
way to check the clauses. I had no immediate success triggering one of the
very few other warnings that use the location of omp_clauses from both Fortran
and C code and hence I went with the nested reductions code.

Thanks for your review!

Best regards,
Frederik

Re: [PATCH 0/2] Add tests to verify OpenACC clause locations

[Thomas Schwinge]

Hi Frederik!

On 2019-12-10T15:23:01+0100, Frederik Harwath  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 " in the commit log, see
.

> 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  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  
>
> 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  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  
>
> 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

[PATCH 0/2] Add tests to verify OpenACC clause locations

[Frederik Harwath]

Hi,

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?

Best regards,
Frederik

Frederik Harwath (2):
  Use clause locations in OpenACC nested reduction warnings
  Add tests to verify OpenACC clause locations

 gcc/omp-low.c  |  2 +-
 gcc/testsuite/gcc.dg/goacc/clause-locations.c  | 17 +
 .../gfortran.dg/goacc/clause-locations.f90 | 18 ++
 3 files changed, 36 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/goacc/clause-locations.c
 create mode 100644 gcc/testsuite/gfortran.dg/goacc/clause-locations.f90

-- 
2.17.1