[Patch] Fortran: %re/%im fixes for OpenMP/OpenACC + gfc_is_simplify_contiguous

[Patch] Fortran: %re/%im fixes for OpenMP/OpenACC + gfc_is_simplify_contiguous

[Tobias Burnus]

[-- Attachment #1: Type: text/plain, Size: 1672 bytes --]

Found when looking at Julian's 3/4 OpenACC patch, which has not
yet been committed (and needs still to be revised a bit.)

The fix (a) avoids an ICE when Julian's patch has been applied.
The patch (b) just makes one error message less confusing.

The testcase shows that only %re/%im run reach the new code as
%kind/%len are != EXPR_VARIABLE. (The error message is slightly
misleading if the 'list item'/'var' is not a variable.)


(a) We need to handle REF_INQUIRY gfc_is_simplify_contiguous.

That function is used for 'is_contiguous(a(:)%re)', but it works
without this patch and simplifies already to .false.
And it is used in openmp.c, which can ICE without this patch.

(b) Just makes the error message nicer - as only EXPR_VARIABLE
reaches that code, only %re and %im are mentioned in the
error message.

(Actually, it is not completely clear whether %re/%im are invalid
or not; I think they should be – but one can argue otherwise.
For OpenMP I just saw that it is tacked internally in Issue 2661,
for OpenACC it is tracked as Issue 346 – but neither has been
discussed, yet.)

OK for mainline?

Tobias

PS: '%re'/'%im' permit accessing the real/imaginary part of a
complex variable as lvalue (in the C++ sense) and also permit
"var(:)%re = 1.0", which real(z) or imag(z) does not permit.

var%kind == kind(var), but derived types also permit
parameterized derived types (PDT), which can use '%foo' for respective
'len' and 'kind' components.

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf

[-- Attachment #2: cmplx-omp-oacc-v2.diff --]
[-- Type: text/x-patch, Size: 4132 bytes --]

Fortran: %re/%im fixes for OpenMP/OpenACC + gfc_is_simplify_contiguous

gcc/fortran/ChangeLog:

	* expr.c (gfc_is_simplify_contiguous): Handle REF_INQUIRY, i.e.
	%im and %re which are EXPR_VARIABLE.
	* openmp.c (resolve_omp_clauses): Diagnose %re/%im explicitly.

gcc/testsuite/ChangeLog:

	* gfortran.dg/goacc-gomp/ref_inquiry.f90: New test.

 gcc/fortran/expr.c                                 |  2 ++
 gcc/fortran/openmp.c                               |  8 +++++
 .../gfortran.dg/goacc-gomp/ref_inquiry.f90         | 42 ++++++++++++++++++++++
 3 files changed, 52 insertions(+)

diff --git a/gcc/fortran/expr.c b/gcc/fortran/expr.c
index 4f456fc629a..92a6700568d 100644
--- a/gcc/fortran/expr.c
+++ b/gcc/fortran/expr.c
@@ -5854,6 +5854,8 @@ gfc_is_simply_contiguous (gfc_expr *expr, bool strict, bool permit_element)
 	part_ref  = ref;
       else if (ref->type == REF_SUBSTRING)
 	return false;
+      else if (ref->type == REF_INQUIRY)
+	return false;
       else if (ref->u.ar.type != AR_ELEMENT)
 	ar = &ref->u.ar;
     }
diff --git a/gcc/fortran/openmp.c b/gcc/fortran/openmp.c
index 797f6c86b62..f0307c801a5 100644
--- a/gcc/fortran/openmp.c
+++ b/gcc/fortran/openmp.c
@@ -5219,6 +5219,14 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses *omp_clauses,
 				&& array_ref->next->type == REF_SUBSTRING)))
 		      gfc_error ("Unexpected substring reference in %s clause "
 				 "at %L", name, &n->where);
+		    else if (array_ref && array_ref->type == REF_INQUIRY)
+		      {
+			gcc_assert (array_ref->u.i == INQUIRY_RE
+				    || array_ref->u.i == INQUIRY_IM);
+			gfc_error ("Unexpected complex-parts designator "
+				   "reference in %s clause at %L",
+				   name, &n->where);
+		      }
 		    else if (!resolved
 			|| n->expr->expr_type != EXPR_VARIABLE
 			|| array_ref->next
diff --git a/gcc/testsuite/gfortran.dg/goacc-gomp/ref_inquiry.f90 b/gcc/testsuite/gfortran.dg/goacc-gomp/ref_inquiry.f90
new file mode 100644
index 00000000000..f92a6632af9
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/goacc-gomp/ref_inquiry.f90
@@ -0,0 +1,42 @@
+implicit none
+type t
+  integer :: i
+  character :: c
+  complex :: z
+end type t
+
+integer :: i
+character(kind=4, len=5) :: c
+complex :: z, zz(5)
+type(t) :: x
+
+print *, is_contiguous(zz(:)%re)
+
+! EXPR_VARIABLE / Cf. also OpenMP spec issue 2661
+!$omp target enter data map(to: z%re)    ! { dg-error "Unexpected complex-parts designator" }
+!$omp target enter data map(to: z%im)    ! { dg-error "Unexpected complex-parts designator" }
+!$omp target enter data map(to: x%z%re)  ! { dg-error "Unexpected complex-parts designator" }
+!$omp target enter data map(to: x%z%im)  ! { dg-error "Unexpected complex-parts designator" }
+
+! Fails differently as it is not a variable (EXPR_VARIABLE)
+!$omp target enter data map(to: i%kind, c%len)     ! { dg-error "not a proper array section" }
+!$omp target enter data map(to: x%i%kind, x%c%len) ! { dg-error "not a proper array section" }
+
+! Likewise for OpenACC
+!$acc enter data copyin(i%kind, c%len)     ! { dg-error "not a proper array section" }
+!$acc enter data copyin(x%i%kind)          ! { dg-error "not a proper array section" }
+!$acc enter data copyin(x%c%len)           ! { dg-error "not a proper array section" }
+!$acc update self(i%kind, c%len)           ! { dg-error "not a proper array section" }
+!$acc update self(x%i%kind)                ! { dg-error "not a proper array section" }
+!$acc update self(x%c%len)                 ! { dg-error "not a proper array section" }
+
+! EXPR_VARIABLE / cf. OpenACC spec issue 346
+!$acc enter data copyin(z%re)   ! { dg-error "Unexpected complex-parts designator" }
+!$acc enter data copyin(z%im)   ! { dg-error "Unexpected complex-parts designator" }
+!$acc enter data copyin(x%z%re) ! { dg-error "Unexpected complex-parts designator" }
+!$acc enter data copyin(x%z%im) ! { dg-error "Unexpected complex-parts designator" }
+!$acc update self(z%re)         ! { dg-error "Unexpected complex-parts designator" }
+!$acc update self(z%im)         ! { dg-error "Unexpected complex-parts designator" }
+!$acc update self(zz%re)
+!$acc update self(zz%im)
+end

Re: [Patch] Fortran: %re/%im fixes for OpenMP/OpenACC + gfc_is_simplify_contiguous

[Thomas Schwinge]

Hi Tobias!

On 2021-02-08T18:50:25+0100, Tobias Burnus <tobias@codesourcery.com> wrote:
> Found when looking at Julian's 3/4 OpenACC patch, which has not
> yet been committed (and needs still to be revised a bit.)
>
> The fix (a) avoids an ICE when Julian's patch has been applied.
> The patch (b) just makes one error message less confusing.
>
> The testcase shows that only %re/%im run reach the new code as
> %kind/%len are != EXPR_VARIABLE. (The error message is slightly
> misleading if the 'list item'/'var' is not a variable.)
>
>
> (a) We need to handle REF_INQUIRY gfc_is_simplify_contiguous.
>
> That function is used for 'is_contiguous(a(:)%re)', but it works
> without this patch and simplifies already to .false.
> And it is used in openmp.c, which can ICE without this patch.
>
> (b) Just makes the error message nicer - as only EXPR_VARIABLE
> reaches that code, only %re and %im are mentioned in the
> error message.

As so often, I can't really comment on the Fortran language/GCC Fortran
front end details.  ;-|

> (Actually, it is not completely clear whether %re/%im are invalid
> or not; I think they should be – but one can argue otherwise.
> For OpenMP I just saw that it is tacked internally in Issue 2661,
> for OpenACC it is tracked as Issue 346 – but neither has been
> discussed, yet.)

Thanks for filing/locating these discussion items for OpenACC/OpenMP
upstream.  May also put these references into the testcases, so that once
these get addressed, we have something to 'grep' for in GCC?

> PS: '%re'/'%im' permit accessing the real/imaginary part of a
> complex variable as lvalue (in the C++ sense) and also permit
> "var(:)%re = 1.0", which real(z) or imag(z) does not permit.
>
> var%kind == kind(var), but derived types also permit
> parameterized derived types (PDT), which can use '%foo' for respective
> 'len' and 'kind' components.

> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/goacc-gomp/ref_inquiry.f90

Originally, the 'goacc-gomp' testsuites have been meant to be used for
testcases related to OpenACC/OpenMP (non-)interoperability, such as
improper nesting etc. (should be the only/major use -- given that
currently there isn't really any OpenACC/OpenMP interoperability).

This one, I'd have put into separate 'gfortran.dg/goacc/ref_inquiry.f90'
and 'gfortran.dg/gomp/ref_inquiry.f90' (everyone cross-referencing the
other, unless using the same filename makes that obvious enough).

> @@ -0,0 +1,42 @@
> +implicit none
> +type t
> +  integer :: i
> +  character :: c
> +  complex :: z
> +end type t
> +
> +integer :: i
> +character(kind=4, len=5) :: c
> +complex :: z, zz(5)
> +type(t) :: x
> +
> +print *, is_contiguous(zz(:)%re)
> +
> +! EXPR_VARIABLE / Cf. also OpenMP spec issue 2661
> +!$omp target enter data map(to: z%re)    ! { dg-error "Unexpected complex-parts designator" }
> +!$omp target enter data map(to: z%im)    ! { dg-error "Unexpected complex-parts designator" }
> +!$omp target enter data map(to: x%z%re)  ! { dg-error "Unexpected complex-parts designator" }
> +!$omp target enter data map(to: x%z%im)  ! { dg-error "Unexpected complex-parts designator" }
> +
> +! Fails differently as it is not a variable (EXPR_VARIABLE)
> +!$omp target enter data map(to: i%kind, c%len)     ! { dg-error "not a proper array section" }
> +!$omp target enter data map(to: x%i%kind, x%c%len) ! { dg-error "not a proper array section" }

I note that 'zz' variants (see below) are not being checked for OpenMP.

> +! Likewise for OpenACC
> +!$acc enter data copyin(i%kind, c%len)     ! { dg-error "not a proper array section" }
> +!$acc enter data copyin(x%i%kind)          ! { dg-error "not a proper array section" }
> +!$acc enter data copyin(x%c%len)           ! { dg-error "not a proper array section" }
> +!$acc update self(i%kind, c%len)           ! { dg-error "not a proper array section" }
> +!$acc update self(x%i%kind)                ! { dg-error "not a proper array section" }
> +!$acc update self(x%c%len)                 ! { dg-error "not a proper array section" }
> +
> +! EXPR_VARIABLE / cf. OpenACC spec issue 346
> +!$acc enter data copyin(z%re)   ! { dg-error "Unexpected complex-parts designator" }
> +!$acc enter data copyin(z%im)   ! { dg-error "Unexpected complex-parts designator" }

Need to check that for 'zz', too?  (See below.)

> +!$acc enter data copyin(x%z%re) ! { dg-error "Unexpected complex-parts designator" }
> +!$acc enter data copyin(x%z%im) ! { dg-error "Unexpected complex-parts designator" }
> +!$acc update self(z%re)         ! { dg-error "Unexpected complex-parts designator" }
> +!$acc update self(z%im)         ! { dg-error "Unexpected complex-parts designator" }
> +!$acc update self(zz%re)
> +!$acc update self(zz%im)
> +end

And for OpenACC, the 'zz' variants do not emit this error message here.
(That's not immediately obvious to me.)

I can see how data mapping of '[...]%re' etc. are problematic (we're
constructing an "incomplete object"?), but 'update' etc. I'd have
expected to work: would just copy the respective "part".

That, again, is however just my gut feeling, without having done any
actual research.


Grüße
 Thomas
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf

Re: [Patch] Fortran: %re/%im fixes for OpenMP/OpenACC + gfc_is_simplify_contiguous

[Tobias Burnus]

[-- Attachment #1: Type: text/plain, Size: 1852 bytes --]

Hi Thomas, hi all

Updated patch. Changes: Testcases split + updated/extended.
OK for mainline?

Regarding the comments:

On 09.02.21 10:45, Thomas Schwinge wrote:
> Thanks for filing/locating these discussion items for OpenACC/OpenMP
> upstream.  May also put these references into the testcases, so that once
> these get addressed, we have something to 'grep' for in GCC?

Actually, they are already in the file. Alternative is to add them as
link, but I am not sure that's better. (I moved them now to the top.)

> I note that 'zz' variants (see below) are not being checked for OpenMP

I have now added them; I had them before but as many checks triggered,
I thought the tests were not really worthwhile.

>> +!$acc update self(zz%re)
>> +!$acc update self(zz%im)
>> +end
> And for OpenACC, the 'zz' variants do not emit this error message here.
> (That's not immediately obvious to me.)

Answer: 'git add' missing. The reason is that with and without
Julian's patch, the error message is different. Without his patch,
the error is:
!$acc update self(zz%re) ! { dg-error "not a proper array section" }

> I can see how data mapping of '[...]%re' etc. are problematic (we're
> constructing an "incomplete object"?), but 'update' etc. I'd have
> expected to work: would just copy the respective "part".
Granted. The array(:)%re access might update too much, but that's not
different to array with strides or with contiguous arrays sections
which contain component reference (and more than one component).

But that's more a question for the spec committee – if it is supposed
to work, the code needs to be updated.

Tobias



-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf

[-- Attachment #2: cmplx-omp-oacc-v3.diff --]
[-- Type: text/x-patch, Size: 6953 bytes --]

Fortran: %re/%im fixes for OpenMP/OpenACC + gfc_is_simplify_contiguous

gcc/fortran/ChangeLog:

	* expr.c (gfc_is_simplify_contiguous): Handle REF_INQUIRY, i.e.
	%im and %re which are EXPR_VARIABLE.
	* openmp.c (resolve_omp_clauses): Diagnose %re/%im explicitly.

gcc/testsuite/ChangeLog:

	* gfortran.dg/goacc/ref_inquiry.f90: New test.
	* gfortran.dg/gomp/ref_inquiry.f90: New test.

 gcc/fortran/expr.c                              |  2 +
 gcc/fortran/openmp.c                            |  8 ++++
 gcc/testsuite/gfortran.dg/goacc/ref_inquiry.f90 | 52 +++++++++++++++++++++++++
 gcc/testsuite/gfortran.dg/gomp/ref_inquiry.f90  | 39 +++++++++++++++++++
 4 files changed, 101 insertions(+)

diff --git a/gcc/fortran/expr.c b/gcc/fortran/expr.c
index 4f456fc629a..92a6700568d 100644
--- a/gcc/fortran/expr.c
+++ b/gcc/fortran/expr.c
@@ -5854,6 +5854,8 @@ gfc_is_simply_contiguous (gfc_expr *expr, bool strict, bool permit_element)
 	part_ref  = ref;
       else if (ref->type == REF_SUBSTRING)
 	return false;
+      else if (ref->type == REF_INQUIRY)
+	return false;
       else if (ref->u.ar.type != AR_ELEMENT)
 	ar = &ref->u.ar;
     }
diff --git a/gcc/fortran/openmp.c b/gcc/fortran/openmp.c
index 797f6c86b62..f0307c801a5 100644
--- a/gcc/fortran/openmp.c
+++ b/gcc/fortran/openmp.c
@@ -5219,6 +5219,14 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses *omp_clauses,
 				&& array_ref->next->type == REF_SUBSTRING)))
 		      gfc_error ("Unexpected substring reference in %s clause "
 				 "at %L", name, &n->where);
+		    else if (array_ref && array_ref->type == REF_INQUIRY)
+		      {
+			gcc_assert (array_ref->u.i == INQUIRY_RE
+				    || array_ref->u.i == INQUIRY_IM);
+			gfc_error ("Unexpected complex-parts designator "
+				   "reference in %s clause at %L",
+				   name, &n->where);
+		      }
 		    else if (!resolved
 			|| n->expr->expr_type != EXPR_VARIABLE
 			|| array_ref->next
diff --git a/gcc/testsuite/gfortran.dg/goacc/ref_inquiry.f90 b/gcc/testsuite/gfortran.dg/goacc/ref_inquiry.f90
new file mode 100644
index 00000000000..274065dae94
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/goacc/ref_inquiry.f90
@@ -0,0 +1,52 @@
+! Check for <var>%re, ...%im, ...%kind, ...%len
+! Cf. also OpenMP's ../gomp/ref_inquiry.f90
+! Cf. OpenACC spec issue 346
+! 
+implicit none
+type t
+  integer :: i
+  character :: c
+  complex :: z
+  complex :: zz(5)
+end type t
+
+integer :: i
+character(kind=4, len=5) :: c
+complex :: z, zz(5)
+type(t) :: x
+
+print *, is_contiguous(zz(:)%re)
+
+! inquiry function; expr_type != EXPR_VARIABLE:
+!$acc enter data copyin(i%kind, c%len)     ! { dg-error "not a proper array section" }
+!$acc enter data copyin(x%i%kind)          ! { dg-error "not a proper array section" }
+!$acc enter data copyin(x%c%len)           ! { dg-error "not a proper array section" }
+!$acc update self(i%kind, c%len)           ! { dg-error "not a proper array section" }
+!$acc update self(x%i%kind)                ! { dg-error "not a proper array section" }
+!$acc update self(x%c%len)                 ! { dg-error "not a proper array section" }
+
+! EXPR_VARIABLE
+!$acc enter data copyin(z%re)    ! { dg-error "Unexpected complex-parts designator" }
+!$acc enter data copyin(z%im)    ! { dg-error "Unexpected complex-parts designator" }
+!$acc enter data copyin(zz%re)   ! { dg-error "not a proper array section" }
+                                 ! { dg-error "Array is not contiguous" "" { target *-*-* } .-1 }
+!$acc enter data copyin(zz%im)   ! { dg-error "not a proper array section" }
+                                 ! { dg-error "Array is not contiguous" "" { target *-*-* } .-1 }
+
+!$acc enter data copyin(x%z%re)  ! { dg-error "Unexpected complex-parts designator" }
+!$acc enter data copyin(x%z%im)  ! { dg-error "Unexpected complex-parts designator" }
+!$acc enter data copyin(x%zz%re) ! { dg-error "not a proper array section" }
+                                 ! { dg-error "Array is not contiguous" "" { target *-*-* } .-1 }
+!$acc enter data copyin(x%zz%im) ! { dg-error "not a proper array section" }
+                                 ! { dg-error "Array is not contiguous" "" { target *-*-* } .-1 }
+
+!$acc update self(z%re)         ! { dg-error "Unexpected complex-parts designator" }
+!$acc update self(z%im)         ! { dg-error "Unexpected complex-parts designator" }
+!$acc update self(zz%re)        ! { dg-error "not a proper array section" }
+!$acc update self(zz%im)        ! { dg-error "not a proper array section" }
+
+!$acc update self(x%z%re)       ! { dg-error "Unexpected complex-parts designator" }
+!$acc update self(x%z%im)       ! { dg-error "Unexpected complex-parts designator" }
+!$acc update self(x%zz%re)      ! { dg-error "is not a proper array section" }
+!$acc update self(x%zz%im)      ! { dg-error "is not a proper array section" }
+end
diff --git a/gcc/testsuite/gfortran.dg/gomp/ref_inquiry.f90 b/gcc/testsuite/gfortran.dg/gomp/ref_inquiry.f90
new file mode 100644
index 00000000000..37461040560
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/gomp/ref_inquiry.f90
@@ -0,0 +1,39 @@
+! Check for <var>%re, ...%im, ...%kind, ...%len
+! Cf. also OpenACC's ../goacc/ref_inquiry.f90
+! Cf. also OpenMP spec issue 2661
+implicit none
+type t
+  integer :: i
+  character :: c
+  complex :: z
+  complex :: zz(5)
+end type t
+
+integer :: i
+character(kind=4, len=5) :: c
+complex :: z, zz(5)
+type(t) :: x
+
+print *, is_contiguous(zz(:)%re)
+
+! inquiry function; expr_type != EXPR_VARIABLE:
+!$omp target enter data map(to: i%kind, c%len)     ! { dg-error "not a proper array section" }
+!$omp target enter data map(to: x%i%kind)          ! { dg-error "not a proper array section" }
+!$omp target enter data map(to: x%c%len)           ! { dg-error "not a proper array section" }
+
+! EXPR_VARIABLE
+!$omp target enter data map(to: z%re)    ! { dg-error "Unexpected complex-parts designator" }
+!$omp target enter data map(to: z%im)    ! { dg-error "Unexpected complex-parts designator" }
+!$omp target enter data map(to: zz%re)   ! { dg-error "not a proper array section" }
+                                         ! { dg-error "Array is not contiguous" "" { target *-*-* } .-1 }
+!$omp target enter data map(to: zz%im)   ! { dg-error "not a proper array section" }
+                                         ! { dg-error "Array is not contiguous" "" { target *-*-* } .-1 }
+
+!$omp target enter data map(to: x%z%re)  ! { dg-error "Unexpected complex-parts designator" }
+!$omp target enter data map(to: x%z%im)  ! { dg-error "Unexpected complex-parts designator" }
+!$omp target enter data map(to: x%zz%re) ! { dg-error "not a proper array section" }
+                                         ! { dg-error "Array is not contiguous" "" { target *-*-* } .-1 }
+!$omp target enter data map(to: x%zz%im) ! { dg-error "not a proper array section" }
+                                         ! { dg-error "Array is not contiguous" "" { target *-*-* } .-1 }
+
+end

Re: [Patch] Fortran: %re/%im fixes for OpenMP/OpenACC + gfc_is_simplify_contiguous

[Thomas Schwinge]

Hi Tobias!

On 2021-02-09T12:41:48+0100, Tobias Burnus <tobias@codesourcery.com> wrote:
> Updated patch. Changes: Testcases split + updated/extended.
> OK for mainline?

I don't have any further comments on the patch itself.

> On 09.02.21 10:45, Thomas Schwinge wrote:
>> Thanks for filing/locating these discussion items for OpenACC/OpenMP
>> upstream.  May also put these references into the testcases, so that once
>> these get addressed, we have something to 'grep' for in GCC?
>
> Actually, they are already in the file.

Heh, thanks, I see -- evidently I haven't been reading carefully...

> Alternative is to add them as
> link, but I am not sure that's better. (I moved them now to the top.)

ACK.  As no doubt you've noticed, I like to put in direct/explicit links
-- but that then occasionally has lead to people external to
OpenACC/OpenMP GitHub reporting that they opened these and got a 404
"Page not found", confusing.

>> I note that 'zz' variants (see below) are not being checked for OpenMP
>
> I have now added them; I had them before but as many checks triggered,
> I thought the tests were not really worthwhile.

(Your call, I suppose -- I just noted the inconistency.)

>>> +!$acc update self(zz%re)
>>> +!$acc update self(zz%im)
>>> +end

>> And for OpenACC, the 'zz' variants do not emit this error message here.
>> (That's not immediately obvious to me.)
>
> Answer: 'git add' missing.

Heh.  ;-)

> The reason is that with and without
> Julian's patch, the error message is different. Without his patch,
> the error is:
> !$acc update self(zz%re) ! { dg-error "not a proper array section" }

Meaning that he'll then to updated these with his patch.  (Julian CCed.)

>> I can see how data mapping of '[...]%re' etc. are problematic (we're
>> constructing an "incomplete object"?), but 'update' etc. I'd have
>> expected to work: would just copy the respective "part".

> Granted. The array(:)%re access might update too much, but that's not
> different to array with strides or with contiguous arrays sections
> which contain component reference (and more than one component).

(Is that indeed allowed to "update too much"?)

> But that's more a question for the spec committee – if it is supposed
> to work, the code needs to be updated.

ACK.


Grüße
 Thomas


> Fortran: %re/%im fixes for OpenMP/OpenACC + gfc_is_simplify_contiguous
>
> gcc/fortran/ChangeLog:
>
>       * expr.c (gfc_is_simplify_contiguous): Handle REF_INQUIRY, i.e.
>       %im and %re which are EXPR_VARIABLE.
>       * openmp.c (resolve_omp_clauses): Diagnose %re/%im explicitly.
>
> gcc/testsuite/ChangeLog:
>
>       * gfortran.dg/goacc/ref_inquiry.f90: New test.
>       * gfortran.dg/gomp/ref_inquiry.f90: New test.
>
>  gcc/fortran/expr.c                              |  2 +
>  gcc/fortran/openmp.c                            |  8 ++++
>  gcc/testsuite/gfortran.dg/goacc/ref_inquiry.f90 | 52 +++++++++++++++++++++++++
>  gcc/testsuite/gfortran.dg/gomp/ref_inquiry.f90  | 39 +++++++++++++++++++
>  4 files changed, 101 insertions(+)
>
> diff --git a/gcc/fortran/expr.c b/gcc/fortran/expr.c
> index 4f456fc629a..92a6700568d 100644
> --- a/gcc/fortran/expr.c
> +++ b/gcc/fortran/expr.c
> @@ -5854,6 +5854,8 @@ gfc_is_simply_contiguous (gfc_expr *expr, bool strict, bool permit_element)
>       part_ref  = ref;
>        else if (ref->type == REF_SUBSTRING)
>       return false;
> +      else if (ref->type == REF_INQUIRY)
> +     return false;
>        else if (ref->u.ar.type != AR_ELEMENT)
>       ar = &ref->u.ar;
>      }
> diff --git a/gcc/fortran/openmp.c b/gcc/fortran/openmp.c
> index 797f6c86b62..f0307c801a5 100644
> --- a/gcc/fortran/openmp.c
> +++ b/gcc/fortran/openmp.c
> @@ -5219,6 +5219,14 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses *omp_clauses,
>                               && array_ref->next->type == REF_SUBSTRING)))
>                     gfc_error ("Unexpected substring reference in %s clause "
>                                "at %L", name, &n->where);
> +                 else if (array_ref && array_ref->type == REF_INQUIRY)
> +                   {
> +                     gcc_assert (array_ref->u.i == INQUIRY_RE
> +                                 || array_ref->u.i == INQUIRY_IM);
> +                     gfc_error ("Unexpected complex-parts designator "
> +                                "reference in %s clause at %L",
> +                                name, &n->where);
> +                   }
>                   else if (!resolved
>                       || n->expr->expr_type != EXPR_VARIABLE
>                       || array_ref->next
> diff --git a/gcc/testsuite/gfortran.dg/goacc/ref_inquiry.f90 b/gcc/testsuite/gfortran.dg/goacc/ref_inquiry.f90
> new file mode 100644
> index 00000000000..274065dae94
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/goacc/ref_inquiry.f90
> @@ -0,0 +1,52 @@
> +! Check for <var>%re, ...%im, ...%kind, ...%len
> +! Cf. also OpenMP's ../gomp/ref_inquiry.f90
> +! Cf. OpenACC spec issue 346
> +!
> +implicit none
> +type t
> +  integer :: i
> +  character :: c
> +  complex :: z
> +  complex :: zz(5)
> +end type t
> +
> +integer :: i
> +character(kind=4, len=5) :: c
> +complex :: z, zz(5)
> +type(t) :: x
> +
> +print *, is_contiguous(zz(:)%re)
> +
> +! inquiry function; expr_type != EXPR_VARIABLE:
> +!$acc enter data copyin(i%kind, c%len)     ! { dg-error "not a proper array section" }
> +!$acc enter data copyin(x%i%kind)          ! { dg-error "not a proper array section" }
> +!$acc enter data copyin(x%c%len)           ! { dg-error "not a proper array section" }
> +!$acc update self(i%kind, c%len)           ! { dg-error "not a proper array section" }
> +!$acc update self(x%i%kind)                ! { dg-error "not a proper array section" }
> +!$acc update self(x%c%len)                 ! { dg-error "not a proper array section" }
> +
> +! EXPR_VARIABLE
> +!$acc enter data copyin(z%re)    ! { dg-error "Unexpected complex-parts designator" }
> +!$acc enter data copyin(z%im)    ! { dg-error "Unexpected complex-parts designator" }
> +!$acc enter data copyin(zz%re)   ! { dg-error "not a proper array section" }
> +                                 ! { dg-error "Array is not contiguous" "" { target *-*-* } .-1 }
> +!$acc enter data copyin(zz%im)   ! { dg-error "not a proper array section" }
> +                                 ! { dg-error "Array is not contiguous" "" { target *-*-* } .-1 }
> +
> +!$acc enter data copyin(x%z%re)  ! { dg-error "Unexpected complex-parts designator" }
> +!$acc enter data copyin(x%z%im)  ! { dg-error "Unexpected complex-parts designator" }
> +!$acc enter data copyin(x%zz%re) ! { dg-error "not a proper array section" }
> +                                 ! { dg-error "Array is not contiguous" "" { target *-*-* } .-1 }
> +!$acc enter data copyin(x%zz%im) ! { dg-error "not a proper array section" }
> +                                 ! { dg-error "Array is not contiguous" "" { target *-*-* } .-1 }
> +
> +!$acc update self(z%re)         ! { dg-error "Unexpected complex-parts designator" }
> +!$acc update self(z%im)         ! { dg-error "Unexpected complex-parts designator" }
> +!$acc update self(zz%re)        ! { dg-error "not a proper array section" }
> +!$acc update self(zz%im)        ! { dg-error "not a proper array section" }
> +
> +!$acc update self(x%z%re)       ! { dg-error "Unexpected complex-parts designator" }
> +!$acc update self(x%z%im)       ! { dg-error "Unexpected complex-parts designator" }
> +!$acc update self(x%zz%re)      ! { dg-error "is not a proper array section" }
> +!$acc update self(x%zz%im)      ! { dg-error "is not a proper array section" }
> +end
> diff --git a/gcc/testsuite/gfortran.dg/gomp/ref_inquiry.f90 b/gcc/testsuite/gfortran.dg/gomp/ref_inquiry.f90
> new file mode 100644
> index 00000000000..37461040560
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/gomp/ref_inquiry.f90
> @@ -0,0 +1,39 @@
> +! Check for <var>%re, ...%im, ...%kind, ...%len
> +! Cf. also OpenACC's ../goacc/ref_inquiry.f90
> +! Cf. also OpenMP spec issue 2661
> +implicit none
> +type t
> +  integer :: i
> +  character :: c
> +  complex :: z
> +  complex :: zz(5)
> +end type t
> +
> +integer :: i
> +character(kind=4, len=5) :: c
> +complex :: z, zz(5)
> +type(t) :: x
> +
> +print *, is_contiguous(zz(:)%re)
> +
> +! inquiry function; expr_type != EXPR_VARIABLE:
> +!$omp target enter data map(to: i%kind, c%len)     ! { dg-error "not a proper array section" }
> +!$omp target enter data map(to: x%i%kind)          ! { dg-error "not a proper array section" }
> +!$omp target enter data map(to: x%c%len)           ! { dg-error "not a proper array section" }
> +
> +! EXPR_VARIABLE
> +!$omp target enter data map(to: z%re)    ! { dg-error "Unexpected complex-parts designator" }
> +!$omp target enter data map(to: z%im)    ! { dg-error "Unexpected complex-parts designator" }
> +!$omp target enter data map(to: zz%re)   ! { dg-error "not a proper array section" }
> +                                         ! { dg-error "Array is not contiguous" "" { target *-*-* } .-1 }
> +!$omp target enter data map(to: zz%im)   ! { dg-error "not a proper array section" }
> +                                         ! { dg-error "Array is not contiguous" "" { target *-*-* } .-1 }
> +
> +!$omp target enter data map(to: x%z%re)  ! { dg-error "Unexpected complex-parts designator" }
> +!$omp target enter data map(to: x%z%im)  ! { dg-error "Unexpected complex-parts designator" }
> +!$omp target enter data map(to: x%zz%re) ! { dg-error "not a proper array section" }
> +                                         ! { dg-error "Array is not contiguous" "" { target *-*-* } .-1 }
> +!$omp target enter data map(to: x%zz%im) ! { dg-error "not a proper array section" }
> +                                         ! { dg-error "Array is not contiguous" "" { target *-*-* } .-1 }
> +
> +end
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf

Re: [Patch] Fortran: %re/%im fixes for OpenMP/OpenACC + gfc_is_simplify_contiguous

[Tobias Burnus]

On 09.02.21 12:58, Thomas Schwinge wrote:
>> Granted. The array(:)%re access might update too much, but that's not
>> different to array with strides or with contiguous arrays sections
>> which contain component reference (and more than one component).
> (Is that indeed allowed to "update too much"?)

Yes - that's the general problem with strides or bit sets;
copying only a subset – and doing so atomically – is not
always possible or feasible.

*OpenACC* 3.1 has for "2.14.4  Update Directive" the restriction:

"Noncontiguous subarrays may appear. It is implementation-specific
  whether noncontiguous regions are updated by using one transfer
  for each contiguous subregion, or whether the noncontiguous data
  is packed, transferred once, and unpacked, or whether one or more
  larger subarrays (no larger than the smallest contiguous region
  that contains the specified subarray) are updated."

For map, I saw that that's the case – but I think Julian's
patch does not handle this correctly for:

type t
   integer :: i, j, k
end type t
type(t) :: A(100)
   ... host(A(:)%j)

I think instead of transferring A(1)%j to A(100)%j, it transfers
all of A(:), i.e. also A(1)%i and A(100)%k :-(

^– Julian?


For OpenMP and map, I recall encountering a code which did do
this for OpenMP (i.e. contiguous subsection). I think it was
related to derived-type 'map', but I do not recall anymore.


Looking at the *OpenMP* 5.1 spec, I see that 'target update' also
allows: "The list items that appear in the to or from clauses
may include array sections with stride expressions."
While for the map clause, there is:
'If a list item is an array section, it must specify contiguous
storage.'

But I did not see a more explicit description how that should be
handled, contrary to the rather explicit description for OpenACC.

Tobias

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf

Re: [Patch] Fortran: %re/%im fixes for OpenMP/OpenACC + gfc_is_simplify_contiguous

[Julian Brown]

On Tue, 9 Feb 2021 13:45:36 +0100
Tobias Burnus <tobias@codesourcery.com> wrote:

> On 09.02.21 12:58, Thomas Schwinge wrote:
> >> Granted. The array(:)%re access might update too much, but that's
> >> not different to array with strides or with contiguous arrays
> >> sections which contain component reference (and more than one
> >> component).  
> > (Is that indeed allowed to "update too much"?)  
> 
> Yes - that's the general problem with strides or bit sets;
> copying only a subset – and doing so atomically – is not
> always possible or feasible.
> 
> *OpenACC* 3.1 has for "2.14.4  Update Directive" the restriction:
> 
> "Noncontiguous subarrays may appear. It is implementation-specific
>   whether noncontiguous regions are updated by using one transfer
>   for each contiguous subregion, or whether the noncontiguous data
>   is packed, transferred once, and unpacked, or whether one or more
>   larger subarrays (no larger than the smallest contiguous region
>   that contains the specified subarray) are updated."
> 
> For map, I saw that that's the case – but I think Julian's
> patch does not handle this correctly for:
> 
> type t
>    integer :: i, j, k
> end type t
> type(t) :: A(100)
>    ... host(A(:)%j)
> 
> I think instead of transferring A(1)%j to A(100)%j, it transfers
> all of A(:), i.e. also A(1)%i and A(100)%k :-(
> 
> ^– Julian?

Yes it will -- but given that A(2)%i and A(99)%k (and all the in-between
values) can legitimately be transferred according to the spec, how much
of a problem is that? In particular, are there situations where this
"over-updating" can lead to incorrect results in a conforming program?

Perhaps the question is, can a user legitimately expect the host and
offloaded versions of some given memory block to hold different data,
like maintaining different data in a cache than the storage backing
that cache? One use-case for that might be double buffering a "single
array" (i.e. the host and device versions of that array). I don't think
that's something we'd want to encourage, though.

I think, rather, that partial updates are an optimisation the user can
use when they know that only part of an array has been updated, so
slight over-copying is harmless.

Thanks,

Julian

Re: [Patch] Fortran: %re/%im fixes for OpenMP/OpenACC + gfc_is_simplify_contiguous

[Julian Brown]

On Tue, 9 Feb 2021 13:45:36 +0100
Tobias Burnus <tobias@codesourcery.com> wrote:

> On 09.02.21 12:58, Thomas Schwinge wrote:
> >> Granted. The array(:)%re access might update too much, but that's
> >> not different to array with strides or with contiguous arrays
> >> sections which contain component reference (and more than one
> >> component).  
> > (Is that indeed allowed to "update too much"?)  
> 
> Yes - that's the general problem with strides or bit sets;
> copying only a subset – and doing so atomically – is not
> always possible or feasible.
> 
> *OpenACC* 3.1 has for "2.14.4  Update Directive" the restriction:
> 
> "Noncontiguous subarrays may appear. It is implementation-specific
>   whether noncontiguous regions are updated by using one transfer
>   for each contiguous subregion, or whether the noncontiguous data
>   is packed, transferred once, and unpacked, or whether one or more
>   larger subarrays (no larger than the smallest contiguous region
>   that contains the specified subarray) are updated."
> 
> For map, I saw that that's the case – but I think Julian's
> patch does not handle this correctly for:
> 
> type t
>    integer :: i, j, k
> end type t
> type(t) :: A(100)
>    ... host(A(:)%j)
> 
> I think instead of transferring A(1)%j to A(100)%j, it transfers
> all of A(:), i.e. also A(1)%i and A(100)%k :-(
> 
> ^– Julian?

Yes it will -- but given that A(2)%i and A(99)%k (and all the in-between
values) can legitimately be transferred according to the spec, how much
of a problem is that? In particular, are there situations where this
"over-updating" can lead to incorrect results in a conforming program?

Perhaps the question is, can a user legitimately expect the host and
offloaded versions of some given memory block to hold different data,
like maintaining different data in a cache than the storage backing
that cache? One use-case for that might be double buffering a "single
array" (i.e. the host and device versions of that array). I don't think
that's something we'd want to encourage, though.

I think, rather, that partial updates are an optimisation the user can
use when they know that only part of an array has been updated, so
slight over-copying is harmless.

Thanks,

Julian

Re: [Patch] Fortran: %re/%im fixes for OpenMP/OpenACC + gfc_is_simplify_contiguous

[Thomas Schwinge]

Hi!

On 2021-02-09T13:05:22+0000, Julian Brown <julian@codesourcery.com> wrote:
> On Tue, 9 Feb 2021 13:45:36 +0100
> Tobias Burnus <tobias@codesourcery.com> wrote:
>
>> On 09.02.21 12:58, Thomas Schwinge wrote:
>> >> Granted. The array(:)%re access might update too much, but that's
>> >> not different to array with strides or with contiguous arrays
>> >> sections which contain component reference (and more than one
>> >> component).
>> > (Is that indeed allowed to "update too much"?)
>>
>> Yes - that's the general problem with strides or bit sets;
>> copying only a subset – and doing so atomically – is not
>> always possible or feasible.
>>
>> *OpenACC* 3.1 has for "2.14.4  Update Directive" the restriction:
>>
>> "Noncontiguous subarrays may appear. It is implementation-specific
>>   whether noncontiguous regions are updated by using one transfer
>>   for each contiguous subregion, or whether the noncontiguous data
>>   is packed, transferred once, and unpacked, or whether one or more
>>   larger subarrays (no larger than the smallest contiguous region
>>   that contains the specified subarray) are updated."
>>
>> For map, I saw that that's the case – but I think Julian's
>> patch does not handle this correctly for:
>>
>> type t
>>    integer :: i, j, k
>> end type t
>> type(t) :: A(100)
>>    ... host(A(:)%j)

So I understand the variants in the quoted OpenACC part as follows.
However I don't claim that I necessarily got all the fine detail right at
the language level (English as well as OpenACC)!  So, please verify.

"Using one transfer for each contiguous subregion":

    for n in 1..100: transfer A(n)%j individually

"Noncontiguous data is packed, transferred once, and unpacked":

    device:
      integer :: tmp(100)
      for n in 1..100: tmp(n) = A(n)%j
    transfer tmp
    host:
      for n in 1..100: A(n)%j = tmp(n)

In this example here, I understand "subarrays (no larger than the
smallest contiguous region that contains the specified subarray)" to
again resolve to 'A(n)%j', so doesn't add other another variant.

This -- per my reading! -- would be different here:

    type t
       integer :: i, j1, j2, k
    end type t
    type(t) :: A(100)
       ... host(A(:)%j1, A(:)%j2)

... where I understand this to mean that each 'A(n)%j1' plus 'A(:)%j2'
may be transferred together: either "using one transfer for each
contiguous subregion":

    for n in 1..100: transfer memory region of A(n)%j1..A(n)%j2 individually

..., or "packed, transferred once, and unpacked":

    device:
      integer :: tmp(2 * 100)
      for n in 1..100:
        tmp(2 * n) = A(n)%j1
        tmp(2 * n + 1) = A(n)%j2
    transfer tmp
    host:
      for n in 1..100:
        A(n)%j1 = tmp(2 * n)
        A(n)%j2 = tmp(2 * n + 1)

I do however not read into this that the following would be valid:

>> I think instead of transferring A(1)%j to A(100)%j, it transfers
>> all of A(:), i.e. also A(1)%i and A(100)%k :-(

I don't think it's appropriate for an 'update' to alter anything else
than the exact 'var's as specified.

> Yes it will -- but given that A(2)%i and A(99)%k (and all the in-between
> values) can legitimately be transferred according to the spec

I don't read it that way, I'm afraid.  :-O

> how much
> of a problem is that? In particular, are there situations where this
> "over-updating" can lead to incorrect results in a conforming program?

In your reading indeed it wouldn't, because the user couldn't expect the
following:

> Perhaps the question is, can a user legitimately expect the host and
> offloaded versions of some given memory block to hold different data,
> like maintaining different data in a cache than the storage backing
> that cache? One use-case for that might be double buffering a "single
> array" (i.e. the host and device versions of that array). I don't think
> that's something we'd want to encourage, though.

I find the wording in the spec rather explicitly, for example: OpenACC
3.1, 2.7 "Data Clauses":

| In all cases, the compiler will allocate and manage a copy of the 'var'
| in the memory of the current device, creating a *visible device copy*
| of that 'var', for data not in shared memory.

Emphasis mine; and I indeed understand this to mean that the user can
"legitimately expect the host and offloaded versions of some given memory
block to hold different data, [...]" (your words from above).

> I think, rather, that partial updates are an optimisation the user can
> use when they know that only part of an array has been updated, so
> slight over-copying is harmless.

Interesting -- and, again: that makes sense in your reading.


So.  Should everybody work through this again, trying to reach consensus?
Do we need to clarify that with OpenACC?


As for OpenMP, Tobias stated:

|On 2021-02-09T13:45:36+0100, Tobias Burnus <tobias@codesourcery.com> wrote:
|> For OpenMP and map, I recall encountering a code which did do
|> this for OpenMP (i.e. contiguous subsection). I think it was
|> related to derived-type 'map', but I do not recall anymore.
|>
|>
|> Looking at the *OpenMP* 5.1 spec, I see that 'target update' also
|> allows: "The list items that appear in the to or from clauses
|> may include array sections with stride expressions."
|> While for the map clause, there is:
|> 'If a list item is an array section, it must specify contiguous
|> storage.'

(I suppose we all agree on that.)

|> But I did not see a more explicit description how that should be
|> handled, contrary to the rather explicit description for OpenACC.

Surprising.  Can some relevant wording (like OpenACC's "visible device
copy") be found elsewhere (in the hundreds of pages...)?

By default, I would've assumed "defensive", so again: "I don't think it's
appropriate for an 'update' to alter anything else than the exact 'var's
as specified." (my words from above)?


Grüße
 Thomas
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf

Re: [Patch] Fortran: %re/%im fixes for OpenMP/OpenACC + gfc_is_simplify_contiguous

[Julian Brown]

On Tue, 9 Feb 2021 16:37:31 +0100
Thomas Schwinge <thomas@codesourcery.com> wrote:

> Hi!
> 
> On 2021-02-09T13:05:22+0000, Julian Brown <julian@codesourcery.com>
> wrote:
> > On Tue, 9 Feb 2021 13:45:36 +0100
> > Tobias Burnus <tobias@codesourcery.com> wrote:
> >  
> >> On 09.02.21 12:58, Thomas Schwinge wrote:  
> >> >> Granted. The array(:)%re access might update too much, but
> >> >> that's not different to array with strides or with contiguous
> >> >> arrays sections which contain component reference (and more
> >> >> than one component).    
> >> > (Is that indeed allowed to "update too much"?)    
> >> 
> >> Yes - that's the general problem with strides or bit sets;
> >> copying only a subset – and doing so atomically – is not
> >> always possible or feasible.
> >> 
> >> *OpenACC* 3.1 has for "2.14.4  Update Directive" the restriction:
> >> 
> >> "Noncontiguous subarrays may appear. It is implementation-specific
> >>   whether noncontiguous regions are updated by using one transfer
> >>   for each contiguous subregion, or whether the noncontiguous data
> >>   is packed, transferred once, and unpacked, or whether one or more
> >>   larger subarrays (no larger than the smallest contiguous region
> >>   that contains the specified subarray) are updated."
> >> 
> >> For map, I saw that that's the case – but I think Julian's
> >> patch does not handle this correctly for:
> >> 
> >> type t
> >>    integer :: i, j, k
> >> end type t
> >> type(t) :: A(100)
> >>    ... host(A(:)%j)  
> 
> So I understand the variants in the quoted OpenACC part as follows.
> However I don't claim that I necessarily got all the fine detail
> right at the language level (English as well as OpenACC)!  So, please
> verify.
> 
> "Using one transfer for each contiguous subregion":
> 
>     for n in 1..100: transfer A(n)%j individually
> 
> "Noncontiguous data is packed, transferred once, and unpacked":
> 
>     device:
>       integer :: tmp(100)
>       for n in 1..100: tmp(n) = A(n)%j
>     transfer tmp
>     host:
>       for n in 1..100: A(n)%j = tmp(n)
> 
> In this example here, I understand "subarrays (no larger than the
> smallest contiguous region that contains the specified subarray)" to
> again resolve to 'A(n)%j', so doesn't add other another variant.
> 
> This -- per my reading! -- would be different here:
> 
>     type t
>        integer :: i, j1, j2, k
>     end type t
>     type(t) :: A(100)
>        ... host(A(:)%j1, A(:)%j2)
> 
> ... where I understand this to mean that each 'A(n)%j1' plus 'A(:)%j2'
> may be transferred together: either "using one transfer for each
> contiguous subregion":

I think you're overthinking it. My reading is that "each contiguous
subregion" just means, e.g. each "j" in:

  !$acc update host(A(:)%j)

although your case with multiple updates to adjacent fields would be
possible too, I guess.

>     for n in 1..100: transfer memory region of A(n)%j1..A(n)%j2
> individually
> 
> ..., or "packed, transferred once, and unpacked":
> 
>     device:
>       integer :: tmp(2 * 100)
>       for n in 1..100:
>         tmp(2 * n) = A(n)%j1
>         tmp(2 * n + 1) = A(n)%j2
>     transfer tmp
>     host:
>       for n in 1..100:
>         A(n)%j1 = tmp(2 * n)
>         A(n)%j2 = tmp(2 * n + 1)

Yes, fine, but...

> I do however not read into this that the following would be valid:
> 
> >> I think instead of transferring A(1)%j to A(100)%j, it transfers
> >> all of A(:), i.e. also A(1)%i and A(100)%k :-(  
> 
> I don't think it's appropriate for an 'update' to alter anything else
> than the exact 'var's as specified.

I disagree here. I think the "one or more larger subarrays" clause is
meant to indicate that a single transfer covering all the data (with
gaps) is acceptable. In terms of implementation -- much of the time
that will likely be more efficient than either transferring lots of
tiny fragments, or copying via an intermediate contiguous buffer, so
your reading of the spec might be shooting ourselves in the foot
somewhat.

One could imagine a heuristic that chooses between one large ("gappy")
transfer and, say, copying via a temporary buffer though, using the
latter if say the density of data transferred is less than some
percentage of the full amount.

> > Yes it will -- but given that A(2)%i and A(99)%k (and all the
> > in-between values) can legitimately be transferred according to the
> > spec  
> 
> I don't read it that way, I'm afraid.  :-O
> 
> > how much
> > of a problem is that? In particular, are there situations where this
> > "over-updating" can lead to incorrect results in a conforming
> > program?  
> 
> In your reading indeed it wouldn't, because the user couldn't expect
> the following:
> 
> > Perhaps the question is, can a user legitimately expect the host and
> > offloaded versions of some given memory block to hold different
> > data, like maintaining different data in a cache than the storage
> > backing that cache? One use-case for that might be double buffering
> > a "single array" (i.e. the host and device versions of that array).
> > I don't think that's something we'd want to encourage, though.  
> 
> I find the wording in the spec rather explicitly, for example: OpenACC
> 3.1, 2.7 "Data Clauses":
> 
> | In all cases, the compiler will allocate and manage a copy of the
> 'var' | in the memory of the current device, creating a *visible
> device copy* | of that 'var', for data not in shared memory.
> 
> Emphasis mine; and I indeed understand this to mean that the user can
> "legitimately expect the host and offloaded versions of some given
> memory block to hold different data, [...]" (your words from above).

I think that's just describing that the copy of the data on the device
is distinct (for pragmatic reasons), not that the user should rely on
it being a separate copy of the data for algorithmic reasons. To do so
would mean a difference in semantics between a program with OpenACC
directives enabled and without, which is I think against the spirit of
the API.

Thanks,

Julian

*ping* – Re: [Patch] Fortran: %re/%im fixes for OpenMP/OpenACC + gfc_is_simplify_contiguous

[Tobias Burnus]

*PING*

this avoids an ICE in gfc_is_simplify_contiguous, which can be at least
triggered with OpenMP/OpenACC.

For %re/%im it outputs a nicer error than an ICE or misleading message.
[Whether %re may be used is still unclear, recently opened OpenMP +
OpenACC spec issues still have to be discussed.]

Tobias

On 09.02.21 12:41, Tobias Burnus wrote:
> Hi Thomas, hi all
>
> Updated patch. Changes: Testcases split + updated/extended.
> OK for mainline?
>
> Regarding the comments:
>
> On 09.02.21 10:45, Thomas Schwinge wrote:
>> Thanks for filing/locating these discussion items for OpenACC/OpenMP
>> upstream.  May also put these references into the testcases, so that
>> once
>> these get addressed, we have something to 'grep' for in GCC?
>
> Actually, they are already in the file. Alternative is to add them as
> link, but I am not sure that's better. (I moved them now to the top.)
>
>> I note that 'zz' variants (see below) are not being checked for OpenMP
>
> I have now added them; I had them before but as many checks triggered,
> I thought the tests were not really worthwhile.
>
>>> +!$acc update self(zz%re)
>>> +!$acc update self(zz%im)
>>> +end
>> And for OpenACC, the 'zz' variants do not emit this error message here.
>> (That's not immediately obvious to me.)
>
> Answer: 'git add' missing. The reason is that with and without
> Julian's patch, the error message is different. Without his patch,
> the error is:
> !$acc update self(zz%re) ! { dg-error "not a proper array section" }
>
>> I can see how data mapping of '[...]%re' etc. are problematic (we're
>> constructing an "incomplete object"?), but 'update' etc. I'd have
>> expected to work: would just copy the respective "part".
> Granted. The array(:)%re access might update too much, but that's not
> different to array with strides or with contiguous arrays sections
> which contain component reference (and more than one component).
>
> But that's more a question for the spec committee – if it is supposed
> to work, the code needs to be updated.
>
> Tobias
>
>
>
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf

Re: [Patch] Fortran: %re/%im fixes for OpenMP/OpenACC + gfc_is_simplify_contiguous

[Paul Richard Thomas]

Hi Tobias,

I think that constitutes an 'obvious' patch. OK for mainline.

Thanks

Paul


On Mon, 8 Feb 2021 at 18:53, Tobias Burnus <tobias@codesourcery.com> wrote:

> Found when looking at Julian's 3/4 OpenACC patch, which has not
> yet been committed (and needs still to be revised a bit.)
>
> The fix (a) avoids an ICE when Julian's patch has been applied.
> The patch (b) just makes one error message less confusing.
>
> The testcase shows that only %re/%im run reach the new code as
> %kind/%len are != EXPR_VARIABLE. (The error message is slightly
> misleading if the 'list item'/'var' is not a variable.)
>
>
> (a) We need to handle REF_INQUIRY gfc_is_simplify_contiguous.
>
> That function is used for 'is_contiguous(a(:)%re)', but it works
> without this patch and simplifies already to .false.
> And it is used in openmp.c, which can ICE without this patch.
>
> (b) Just makes the error message nicer - as only EXPR_VARIABLE
> reaches that code, only %re and %im are mentioned in the
> error message.
>
> (Actually, it is not completely clear whether %re/%im are invalid
> or not; I think they should be – but one can argue otherwise.
> For OpenMP I just saw that it is tacked internally in Issue 2661,
> for OpenACC it is tracked as Issue 346 – but neither has been
> discussed, yet.)
>
> OK for mainline?
>
> Tobias
>
> PS: '%re'/'%im' permit accessing the real/imaginary part of a
> complex variable as lvalue (in the C++ sense) and also permit
> "var(:)%re = 1.0", which real(z) or imag(z) does not permit.
>
> var%kind == kind(var), but derived types also permit
> parameterized derived types (PDT), which can use '%foo' for respective
> 'len' and 'kind' components.
>
> -----------------
> Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München
> Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank
> Thürauf
>


-- 
"If you can't explain it simply, you don't understand it well enough" -
Albert Einstein