Hi Thomas, hi all,
(Updated patch attached – only changes the goacc testcases.
I intent to commit the patch tomorrow, unless more comments come up.)
On 10.03.22 10:00, Thomas Schwinge wrote:
[OpenACC testcases:]
I recently added that checking in ... [...] to document the status quo.
(1), as you have proposed, add '-O0' (but with source code comment, please);
[...]
..., or (2): update the test cases [...] (rationale: the test cases
haven't originally been written for the '-Wuninitialized' diagnostics; [...]
(3): duplicate the test cases to account for both (1) and (2)
[...] (3), would be my approach.
Attached patch does (3). I also remove the code tweaking, added in previous
patch. - But added a bunch of comments.
And I have to admit that I did not realize that the -Wuninitialized was only
added later. (I did not expect that new flags get added to existing patches.)
** I am actually not sure whether 'acc update(b)' will/should map a
previous allocated variable - or whether it should. [...]
testcases.
Should be: "previously *un*allocated" (+ Thomas s%...%.. comments).
I don't quickly dig that, sorry. Do we need to first clarify that with
OpenACC Technical Committee, or is this just a GCC/OpenACC implementation
issue?
That's mostly an OpenACC spec question.
But I did not check what the spec says. Thus, I don't know
* whether the spec needs to be improved
* what the implications are on the implementation.
I assume that the implementation for OpenMP does also make sense for
OpenACC (i.e. either works as required or does more but in a sensible
way) - but I don't know.
I think either/both of us should check the OpenACC spec.
* * *
On the OpenMP side (clarified in 5.1 or 5.2):
* Map first an unallocated allocatable
=> That one is regarded as mapped/present
* Allocate it and map it again (e.g. implicit/explicit
mapping for a target region)
=> Spec: Implementation may or may not update the item.
However, (only) with 'always' the spec guarantees that it
will also show up as allocated on the device.
=> Sentiment: should also work without 'always' modifier
if previously unallocated. (Discussion postponed.)
* I have not checked 'update' but I think it behaves
like 'map(always,to/from:' (except for ref counting)
OpenMP GCC implementation: there is a known issue for
scalar allocatables (PR96668). It does work for arrays
(also without 'always') - and 'omp update' has not been
checked. (Ref counting issues?)
OpenACC GCC implementation: I think this code is shared
with OpenMP and, thus, works likewise. But I have have
also not checked this.
Tobias
-----------------
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
Fortran: OpenMP/OpenACC avoid uninit access in size calc for mapping
gcc/fortran/ChangeLog:
* trans-openmp.cc (gfc_trans_omp_clauses, gfc_omp_finish_clause):
Obtain size for mapping only if allocatable array is allocated.
gcc/testsuite/ChangeLog:
* gfortran.dg/goacc/array-with-dt-1.f90: Update/add comments;
remove dg-warning for 'is used uninitialized'.
* gfortran.dg/goacc/pr93464.f90: Likewise.
* gfortran.dg/goacc/array-with-dt-1a.f90: New; copied from
gfortran.dg/goacc/array-with-dt-1.f90 but run with -O0. Update
dg-warning for 'may be used uninitialized'.
* gfortran.dg/goacc/pr93464-2.f90: Likewise; copied from
gfortran.dg/goacc/pr93464.f90.
gcc/fortran/trans-openmp.cc | 6 +++--
.../gfortran.dg/goacc/array-with-dt-1.f90 | 18 ++++++++++++---
.../gfortran.dg/goacc/array-with-dt-1a.f90 | 27 ++++++++++++++++++++++
gcc/testsuite/gfortran.dg/goacc/pr93464-2.f90 | 26 +++++++++++++++++++++
gcc/testsuite/gfortran.dg/goacc/pr93464.f90 | 12 ++++++----
5 files changed, 80 insertions(+), 9 deletions(-)
diff --git a/gcc/fortran/trans-openmp.cc b/gcc/fortran/trans-openmp.cc
index 4d56a771349..fad76a4791f 100644
--- a/gcc/fortran/trans-openmp.cc
+++ b/gcc/fortran/trans-openmp.cc
@@ -1597,7 +1597,8 @@ gfc_omp_finish_clause (tree c, gimple_seq *pre_p, bool openacc)
tree size = create_tmp_var (gfc_array_index_type);
tree elemsz = TYPE_SIZE_UNIT (gfc_get_element_type (type));
elemsz = fold_convert (gfc_array_index_type, elemsz);
- if (GFC_TYPE_ARRAY_AKIND (type) == GFC_ARRAY_POINTER
+ if (GFC_TYPE_ARRAY_AKIND (type) == GFC_ARRAY_ALLOCATABLE
+ || GFC_TYPE_ARRAY_AKIND (type) == GFC_ARRAY_POINTER
|| GFC_TYPE_ARRAY_AKIND (type) == GFC_ARRAY_POINTER_CONT)
{
stmtblock_t cond_block;
@@ -3208,7 +3209,8 @@ gfc_trans_omp_clauses (stmtblock_t *block, gfc_omp_clauses *clauses,
/* We have to check for n->sym->attr.dimension because
of scalar coarrays. */
- if (n->sym->attr.pointer && n->sym->attr.dimension)
+ if ((n->sym->attr.pointer || n->sym->attr.allocatable)
+ && n->sym->attr.dimension)
{
stmtblock_t cond_block;
tree size
diff --git a/gcc/testsuite/gfortran.dg/goacc/array-with-dt-1.f90 b/gcc/testsuite/gfortran.dg/goacc/array-with-dt-1.f90
index 136e42acd59..fb27f6e4058 100644
--- a/gcc/testsuite/gfortran.dg/goacc/array-with-dt-1.f90
+++ b/gcc/testsuite/gfortran.dg/goacc/array-with-dt-1.f90
@@ -1,15 +1,27 @@
! { dg-additional-options -Wuninitialized }
+! Purpose of this testcase (from the commit log):
+! This patch fixes lowering of derived-type mappings which select elements
+! of arrays of derived types, and similar. These would previously lead
+! to ICEs.
+
+! This testcase does not show any uninitialized warnings when compiled with -O
+! (as done). For -O0, see testcase file 'array-with-dt-1a.f90'.
+
type t
integer, allocatable :: A(:,:)
end type t
type(t), allocatable :: b(:)
-! { dg-note {'b' declared here} {} { target *-*-* } .-1 }
+
+! Remark: Semantically, the following line requires that 'b'
+! is already present on the device.
!$acc update host(b)
-! { dg-warning {'b\.dim\[0\]\.ubound' is used uninitialized} {} { target *-*-* } .-1 }
-! { dg-warning {'b\.dim\[0\]\.lbound' is used uninitialized} {} { target *-*-* } .-2 }
+
+! Remark: Semantically, the following lines require that b is allocated
+! and present on the device. The last line also requires the same for 'A'.
+
!$acc update host(b(:))
!$acc update host(b(1)%A)
!$acc update host(b(1)%A(:,:))
diff --git a/gcc/testsuite/gfortran.dg/goacc/array-with-dt-1a.f90 b/gcc/testsuite/gfortran.dg/goacc/array-with-dt-1a.f90
new file mode 100644
index 00000000000..8beaeb45e2e
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/goacc/array-with-dt-1a.f90
@@ -0,0 +1,27 @@
+! { dg-additional-options "-Wuninitialized -O0" }
+!
+! With -O0 only, 'may be uninitalized' warnings show up.
+! For the original testcase, compiled with '-O',
+! see testcase file 'array-with-dt-1a.f90'
+
+type t
+ integer, allocatable :: A(:,:)
+end type t
+
+type(t), allocatable :: b(:)
+! { dg-note {'b' declared here} {} { target *-*-* } .-1 }
+
+! Remark: Semantically, the following line requires that 'b'
+! is already present on the device.
+
+!$acc update host(b)
+! { dg-warning {'b\.dim\[0\]\.ubound' may be used uninitialized} {} { target *-*-* } .-1 }
+! { dg-warning {'b\.dim\[0\]\.lbound' may be used uninitialized} {} { target *-*-* } .-2 }
+
+! Remark: Semantically, the following lines require that b is allocated
+! and present on the device. The last line also requires the same for 'A'.
+
+!$acc update host(b(:))
+!$acc update host(b(1)%A)
+!$acc update host(b(1)%A(:,:))
+end
diff --git a/gcc/testsuite/gfortran.dg/goacc/pr93464-2.f90 b/gcc/testsuite/gfortran.dg/goacc/pr93464-2.f90
new file mode 100644
index 00000000000..5da6a4eb7b0
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/goacc/pr93464-2.f90
@@ -0,0 +1,26 @@
+! { dg-additional-options "-Wuninitialized -O0" }
+!
+! PR fortran/93464
+!
+! Contributed by G. Steinmetz
+!
+! Did before ICE in gfc_omp_check_optional_argument
+
+! Additionally, check for uninitialized warnings. There are
+! none with -O (cf. original testcase file 'pr93464.f90').
+! For -O0, see below:
+
+program p
+ character :: c(2) = 'a'
+ character, allocatable :: z(:)
+ ! { dg-note {'z' declared here} {} { target *-*-* } .-1 }
+ !$acc parallel
+ ! { dg-warning {'z\.dim\[0\]\.ubound' may be used uninitialized} {} { target *-*-* } .-1 }
+ ! { dg-warning {'z\.dim\[0\]\.lbound' may be used uninitialized} {} { target *-*-* } .-2 }
+ !$omp target
+ ! Remark: As run-time check, required either 'c' being allocated or if(allocated(c)':
+ z = c
+ !$acc end parallel
+ !$omp end target
+ print *, z
+end
diff --git a/gcc/testsuite/gfortran.dg/goacc/pr93464.f90 b/gcc/testsuite/gfortran.dg/goacc/pr93464.f90
index c92f1d3d8b2..1b5ca7d1c76 100644
--- a/gcc/testsuite/gfortran.dg/goacc/pr93464.f90
+++ b/gcc/testsuite/gfortran.dg/goacc/pr93464.f90
@@ -1,17 +1,21 @@
+! { dg-additional-options -Wuninitialized }
+!
! PR fortran/93464
!
! Contributed by G. Steinmetz
+!
+! Did before ICE in gfc_omp_check_optional_argument
-! { dg-additional-options -Wuninitialized }
+! Additionally, check for uninitialized warnings.
+! Compiled with -O (as done here), no show up;
+! for -O0, see testcase file 'pr93464-1.f90'.
program p
character :: c(2) = 'a'
character, allocatable :: z(:)
- ! { dg-note {'z' declared here} {} { target *-*-* } .-1 }
!$acc parallel
- ! { dg-warning {'z\.dim\[0\]\.ubound' is used uninitialized} {} { target *-*-* } .-1 }
- ! { dg-warning {'z\.dim\[0\]\.lbound' is used uninitialized} {} { target *-*-* } .-2 }
!$omp target
+ ! Remark: As run-time check, required either 'c' being allocated or if(allocated(c)':
z = c
!$acc end parallel
!$omp end target
