Re: [Patch] OpenMP/Fortran: 'target update' with DT components (was: [Patch] OpenMP/Fortran: 'target update' with strides + DT components)

2022-11-03 Thread Jakub Jelinek via Gcc-patches 
On Thu, Nov 03, 2022 at 02:35:03PM +0100, Tobias Burnus wrote:
> On 03.11.22 13:44, Jakub Jelinek wrote:
> > [...]
> > Otherwise LGTM, assuming it actually works correctly.
> > 
> > I don't remember support for non-contiguous copying to/from devices
> > being actually added, [...] And I think it is not ok to copy bytes
> > that aren't requested to be copied.
> 
> I have now removed that stride support and only kept the bug fix and the
> DT component parts of the patch.
> 
> The only code change is to remove the stride check disabling in
> openmp.cc and in one testcase, to remove the stride part.
> 
> I will commit it as attached, unless there are further comments (or the
> just started reg testing shows that something does not work).
> 
> Tobias
> 
> PS: For strides, I now filed: PR middle-end/107517 "[OpenMP][5.0]
> 'target update' with strides — for C/C++ and Fortran"
> https://gcc.gnu.org/PR107517
> -
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
> München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
> Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
> München, HRB 106955

> OpenMP/Fortran: 'target update' with DT components
> 
> OpenMP 5.0 permits to use arrays with derived type components for the list
> items to the 'from'/'to' clauses of the 'target update' directive.
> 
> gcc/fortran/ChangeLog:
> 
>   * openmp.cc (gfc_match_omp_clauses): Permit derived types for
>   the 'to' and 'from' clauses of 'target update'.
>   * trans-openmp.cc (gfc_trans_omp_clauses): Fixes for
>   derived-type changes; fix size for scalars.
> 
> libgomp/ChangeLog:
> 
>   * testsuite/libgomp.fortran/target-11.f90: New test.
>   * testsuite/libgomp.fortran/target-13.f90: New test.

LGTM, thanks.

Jakub

[Patch] OpenMP/Fortran: 'target update' with DT components (was: [Patch] OpenMP/Fortran: 'target update' with strides + DT components)

2022-11-03 Thread Tobias Burnus 

On 03.11.22 13:44, Jakub Jelinek wrote:

[...]
Otherwise LGTM, assuming it actually works correctly.

I don't remember support for non-contiguous copying to/from devices
being actually added, [...] And I think it is not ok to copy bytes
that aren't requested to be copied.


I have now removed that stride support and only kept the bug fix and the
DT component parts of the patch.

The only code change is to remove the stride check disabling in
openmp.cc and in one testcase, to remove the stride part.

I will commit it as attached, unless there are further comments (or the
just started reg testing shows that something does not work).

Tobias

PS: For strides, I now filed: PR middle-end/107517 "[OpenMP][5.0]
'target update' with strides — for C/C++ and Fortran"
https://gcc.gnu.org/PR107517
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955
OpenMP/Fortran: 'target update' with DT components

OpenMP 5.0 permits to use arrays with derived type components for the list
items to the 'from'/'to' clauses of the 'target update' directive.

gcc/fortran/ChangeLog:

	* openmp.cc (gfc_match_omp_clauses): Permit derived types for
	the 'to' and 'from' clauses of 'target update'.
	* trans-openmp.cc (gfc_trans_omp_clauses): Fixes for
	derived-type changes; fix size for scalars.

libgomp/ChangeLog:

	* testsuite/libgomp.fortran/target-11.f90: New test.
	* testsuite/libgomp.fortran/target-13.f90: New test.

 gcc/fortran/openmp.cc   |  10 +-
 gcc/fortran/trans-openmp.cc |   9 +-
 libgomp/testsuite/libgomp.fortran/target-11.f90 |  75 +++
 libgomp/testsuite/libgomp.fortran/target-13.f90 | 159 
 4 files changed, 246 insertions(+), 7 deletions(-)

diff --git a/gcc/fortran/openmp.cc b/gcc/fortran/openmp.cc
index 653c43f79ff..e0e3b52ad57 100644
--- a/gcc/fortran/openmp.cc
+++ b/gcc/fortran/openmp.cc
@@ -2499,9 +2499,10 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, const omp_mask mask,
 	  true) == MATCH_YES)
 	continue;
 	  if ((mask & OMP_CLAUSE_FROM)
-	  && gfc_match_omp_variable_list ("from (",
+	  && (gfc_match_omp_variable_list ("from (",
 	  >lists[OMP_LIST_FROM], false,
-	  NULL, , true) == MATCH_YES)
+	  NULL, , true, true)
+		  == MATCH_YES))
 	continue;
 	  break;
 	case 'g':
@@ -3436,9 +3437,10 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, const omp_mask mask,
 		continue;
 	}
 	  else if ((mask & OMP_CLAUSE_TO)
-	  && gfc_match_omp_variable_list ("to (",
+	  && (gfc_match_omp_variable_list ("to (",
 	  >lists[OMP_LIST_TO], false,
-	  NULL, , true) == MATCH_YES)
+	  NULL, , true, true)
+		  == MATCH_YES))
 	continue;
 	  break;
 	case 'u':
diff --git a/gcc/fortran/trans-openmp.cc b/gcc/fortran/trans-openmp.cc
index 9bd4e6c7e1b..4bfdf85cd9b 100644
--- a/gcc/fortran/trans-openmp.cc
+++ b/gcc/fortran/trans-openmp.cc
@@ -3626,7 +3626,10 @@ gfc_trans_omp_clauses (stmtblock_t *block, gfc_omp_clauses *clauses,
 		  gcc_unreachable ();
 		}
 	  tree node = build_omp_clause (input_location, clause_code);
-	  if (n->expr == NULL || n->expr->ref->u.ar.type == AR_FULL)
+	  if (n->expr == NULL
+		  || (n->expr->ref->type == REF_ARRAY
+		  && n->expr->ref->u.ar.type == AR_FULL
+		  && n->expr->ref->next == NULL))
 		{
 		  tree decl = gfc_trans_omp_variable (n->sym, false);
 		  if (gfc_omp_privatize_by_reference (decl))
@@ -3666,13 +3669,13 @@ gfc_trans_omp_clauses (stmtblock_t *block, gfc_omp_clauses *clauses,
 		{
 		  tree ptr;
 		  gfc_init_se (, NULL);
-		  if (n->expr->ref->u.ar.type == AR_ELEMENT)
+		  if (n->expr->rank == 0)
 		{
 		  gfc_conv_expr_reference (, n->expr);
 		  ptr = se.expr;
 		  gfc_add_block_to_block (block, );
 		  OMP_CLAUSE_SIZE (node)
-			= TYPE_SIZE_UNIT (TREE_TYPE (ptr));
+			= TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (ptr)));
 		}
 		  else
 		{
diff --git a/libgomp/testsuite/libgomp.fortran/target-11.f90 b/libgomp/testsuite/libgomp.fortran/target-11.f90
new file mode 100644
index 000..b0faa2e620d
--- /dev/null
+++ b/libgomp/testsuite/libgomp.fortran/target-11.f90
@@ -0,0 +1,75 @@
+! Based on libgomp.c/target-23.c
+
+! { dg-additional-options "-fdump-tree-original" }
+! { dg-final { scan-tree-dump "omp target update to\\(xxs\\\[3\\\] \\\[len: 2\\\]\\)" "original" } }
+! { dg-final { scan-tree-dump "omp target update to\\(s\\.s \\\[len: 4\\\]\\)" "original" } }
+! { dg-final { scan-tree-dump "omp target update from\\(s\\.s \\\[len: 4\\\]\\)" "original" } }
+
+module m
+  implicit none
+  type S_type
+integer s
+integer, pointer :: u(:) => null()
+integer :: v(0:4)
+  end type S_type
+  integer, volatile :: z
+end module m
+
+program main
+  use m
+  implicit none
+

Re: [Patch] OpenMP/Fortran: 'target update' with strides + DT components

2022-11-03 Thread Jakub Jelinek via Gcc-patches 
On Mon, Oct 31, 2022 at 03:46:25PM +0100, Tobias Burnus wrote:
> OpenMP/Fortran: 'target update' with strides + DT components
> 
> OpenMP 5.0 permits to use arrays with strides and derived
> type components for the list items to the 'from'/'to' clauses
> of the 'target update' directive.
> 
> gcc/fortran/ChangeLog:
> 
>   * openmp.cc (gfc_match_omp_clauses): Permit derived types.
>   (resolve_omp_clauses):Accept noncontiguous
>   arrays.

Formatting.  Missing space before Accept and arrays. could fit on the
same line.

>   * trans-openmp.cc (gfc_trans_omp_clauses): Fixes for
>   derived-type changes; fix size for scalars.
> 
> libgomp/ChangeLog:
> 
>   * testsuite/libgomp.fortran/target-11.f90: New test.
>   * testsuite/libgomp.fortran/target-13.f90: New test.

Otherwise LGTM, assuming it actually works correctly.

I don't remember support for non-contiguous copying to/from devices
being actually added, on the library side we certainly have
omp_target_memcpy_rect which under the hood just does multiple copies
of the contiguous subparts, but I don't remember something similar
done in GOMP_target_update.  And I think it is not ok to copy bytes
that aren't requested to be copied.

Jakub

[Patch] OpenMP/Fortran: 'target update' with strides + DT components

2022-10-31 Thread Tobias Burnus 

I recently saw that gfortran does not support derived type components
with 'target update', an OpenMP 5.0 feature.

When adding it, I also found out that strides where not handled. There
is probably some room of improvement about what to copy and what not,
but copying too much should be fine.

Build + (reg)tested on x86_64-gnu-linux without offloading configured
+ libgomp tested on x86_64-gnu-linux with nvptx offloading.
OK for mainline?

 * * *

PS: Follow-up work items:
* Strides: OpenMP seemingly permits also 'a%b([1,6,19,12])' as
  long as the first index has the lowest address. – And also
  'a%b(:)%c' is permitted – both not handled in this patch
  (and rejected with a compile-time error)
* There seems to be some problems with 'alloc' with pointers
  and allocatables in components – but I have not rechecked.
* For allocatables, 'target update' needs to do a deep mapping;
  I need to check whether that's the case.
Note for the last two: allocatable components only works OG11/OG12
and I urgently need to cleanup + (re)submit that patch to mainline.
(It came too late for GCC 12.)

* There might be also some issue mapping/refcounting, which I have not
  investigated - affecting the 'target exit data' of target-11.f90.

PPS: I intent to file at least one/some PRs about those issues, unless
I can fix them quickly.
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955
OpenMP/Fortran: 'target update' with strides + DT components

OpenMP 5.0 permits to use arrays with strides and derived
type components for the list items to the 'from'/'to' clauses
of the 'target update' directive.

gcc/fortran/ChangeLog:

	* openmp.cc (gfc_match_omp_clauses): Permit derived types.
	(resolve_omp_clauses):Accept noncontiguous
	arrays.
	* trans-openmp.cc (gfc_trans_omp_clauses): Fixes for
	derived-type changes; fix size for scalars.

libgomp/ChangeLog:

	* testsuite/libgomp.fortran/target-11.f90: New test.
	* testsuite/libgomp.fortran/target-13.f90: New test.

 gcc/fortran/openmp.cc   |  19 ++-
 gcc/fortran/trans-openmp.cc |   9 +-
 libgomp/testsuite/libgomp.fortran/target-11.f90 |  75 +++
 libgomp/testsuite/libgomp.fortran/target-13.f90 | 162 
 4 files changed, 256 insertions(+), 9 deletions(-)

diff --git a/gcc/fortran/openmp.cc b/gcc/fortran/openmp.cc
index 653c43f79ff..2daed74be72 100644
--- a/gcc/fortran/openmp.cc
+++ b/gcc/fortran/openmp.cc
@@ -2499,9 +2499,10 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, const omp_mask mask,
 	  true) == MATCH_YES)
 	continue;
 	  if ((mask & OMP_CLAUSE_FROM)
-	  && gfc_match_omp_variable_list ("from (",
+	  && (gfc_match_omp_variable_list ("from (",
 	  >lists[OMP_LIST_FROM], false,
-	  NULL, , true) == MATCH_YES)
+	  NULL, , true, true)
+		  == MATCH_YES))
 	continue;
 	  break;
 	case 'g':
@@ -3436,9 +3437,10 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, const omp_mask mask,
 		continue;
 	}
 	  else if ((mask & OMP_CLAUSE_TO)
-	  && gfc_match_omp_variable_list ("to (",
+	  && (gfc_match_omp_variable_list ("to (",
 	  >lists[OMP_LIST_TO], false,
-	  NULL, , true) == MATCH_YES)
+	  NULL, , true, true)
+		  == MATCH_YES))
 	continue;
 	  break;
 	case 'u':
@@ -7585,8 +7587,11 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses *omp_clauses,
 			   Only raise an error here if we're really sure the
 			   array isn't contiguous.  An expression such as
 			   arr(-n:n,-n:n) could be contiguous even if it looks
-			   like it may not be.  */
+			   like it may not be.
+			   And OpenMP's 'target update' permits strides for
+			   the to/from clause. */
 			if (code->op != EXEC_OACC_UPDATE
+			&& code->op != EXEC_OMP_TARGET_UPDATE
 			&& list != OMP_LIST_CACHE
 			&& list != OMP_LIST_DEPEND
 			&& !gfc_is_simply_contiguous (n->expr, false, true)
@@ -7630,7 +7635,9 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses *omp_clauses,
 			int i;
 			gfc_array_ref *ar = >u.ar;
 			for (i = 0; i < ar->dimen; i++)
-			  if (ar->stride[i] && code->op != EXEC_OACC_UPDATE)
+			  if (ar->stride[i]
+			  && code->op != EXEC_OACC_UPDATE
+			  && code->op != EXEC_OMP_TARGET_UPDATE)
 			{
 			  gfc_error ("Stride should not be specified for "
 	 "array section in %s clause at %L",
diff --git a/gcc/fortran/trans-openmp.cc b/gcc/fortran/trans-openmp.cc
index 9bd4e6c7e1b..4bfdf85cd9b 100644
--- a/gcc/fortran/trans-openmp.cc
+++ b/gcc/fortran/trans-openmp.cc
@@ -3626,7 +3626,10 @@ gfc_trans_omp_clauses (stmtblock_t *block, gfc_omp_clauses *clauses,
 		  gcc_unreachable ();
 		}
 	  tree node = build_omp_clause (input_location, clause_code);
-	  if (n->expr == NULL || n->expr->ref->u.ar.type