Hi Tobias!
On 2022-03-08T15:25:07+0100, Tobias Burnus <tob...@codesourcery.com> wrote:
> found when working on the deep-mapping patch* with OpenMP code
> (and part of that patch) but it already shows up in an existing
> OpenACC testcase. I think it makes sense to fix it already for GCC 12.
>
> Problem: Also for unallocated allocatables, their size was
> calculated - the 'if(desc.data == NULL)' check was only added
> for pointers.
>
> Result after the patch: When compiling with -O (which is the default
> for goacc.exp), the warning now disappears. Thus, I now use '-O0'
> and the previous "is uninitialized" is now "may be uninitialized".
I recently added that checking in
commit 4bd8b1e881f0c26a5103cd1919809b3d63b60ef2
"Document current '-Wuninitialized'/'-Wmaybe-uninitialized' diagnostics
for OpenACC test cases", to document the status quo.
I'll leave it to you to decide what is more appropriate: (1), as you have
proposed, add '-O0' (but with source code comment, please); something
like:
! { dg-additional-options -Wuninitialized }
+! Trigger "may be used uninitialized".
+! { dg-additional-options -O0 }
..., or (2): update the test cases to simply reflect diagnostics that are
now (no longer) seen with (default) '-O' (rationale: the test cases
haven't originally been written for the '-Wuninitialized' diagnostics;
that's just tested additionally, and using '-O0' instead of '-O' may be
disturbing what they originally meant to test?), or (3): duplicate the
test cases to account for both (1) and (2) (in other words: write
dedicated test cases for your GCC/Fortran front end changes (for example,
based on the ones you've modified here), and for the existing test cases
apply (2)). The latter, (3), would be my approach.
> Unrelated to the patch and the testcase, I added some
> 'allocate'**/'if(allocated())' to the testcase - as otherwise
> uninit vars would be accessed. (Not relevant for the warning
> or the patch - but I prefer no invalid code in testcases,
> if it can be avoided.)
Agreed in principle, but again: I don't know what these test cases
originally have been testing?
> OK for mainline?
I can't comment on the GCC/Fortran front end changes -- so unless
somebody else speaks up, that's an implicit approval for those, I
suppose. ;-)
> Tobias
> * https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591144.html
> ** I am actually not sure whether 'acc update(b)' will/should map a
> previous allocated variable - or whether it should.
(Are the typos here: in "will/should map": 's%map%update', and in
"or whether it should": 's%should%shouldn't'?)
> But that's
> unrelated to this bug fix. See also: https://gcc.gnu.org/PR96668
> for the re-mapping in OpenMP (works for arrays but not scalars).
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?
Grüße
Thomas
> 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: Run with -O0 and
> update dg-warning.
> * gfortran.dg/goacc/pr93464.f90: Likewise.
>
> gcc/fortran/trans-openmp.cc | 6 ++++--
> gcc/testsuite/gfortran.dg/goacc/array-with-dt-1.f90 | 12 +++++++++---
> gcc/testsuite/gfortran.dg/goacc/pr93464.f90 | 8 ++++----
> 3 files changed, 17 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..f6880238c89 100644
> --- a/gcc/testsuite/gfortran.dg/goacc/array-with-dt-1.f90
> +++ b/gcc/testsuite/gfortran.dg/goacc/array-with-dt-1.f90
> @@ -1,4 +1,4 @@
> -! { dg-additional-options -Wuninitialized }
> +! { dg-additional-options "-Wuninitialized -O0" }
>
> type t
> integer, allocatable :: A(:,:)
> @@ -8,9 +8,15 @@ type(t), allocatable :: b(:)
> ! { dg-note {'b' declared here} {} { target *-*-* } .-1 }
>
> !$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 }
> +! { dg-warning {'b\.dim\[0\]\.ubound' may be used uninitialized} {} { target
> *-*-* } .-1 }
> +! { dg-warning {'b\.dim\[0\]\.lbound' may be used uninitialized} {} { target
> *-*-* } .-2 }
> +
> +allocate(b(1))
> +!$acc update host(b)
> !$acc update host(b(:))
> +
> +!$acc update host(b(1)%A)
> +allocate(b(1)%A(1,1))
> !$acc update host(b(1)%A)
> !$acc update host(b(1)%A(:,:))
> end
> diff --git a/gcc/testsuite/gfortran.dg/goacc/pr93464.f90
> b/gcc/testsuite/gfortran.dg/goacc/pr93464.f90
> index c92f1d3d8b2..18531abdf77 100644
> --- a/gcc/testsuite/gfortran.dg/goacc/pr93464.f90
> +++ b/gcc/testsuite/gfortran.dg/goacc/pr93464.f90
> @@ -2,17 +2,17 @@
> !
> ! Contributed by G. Steinmetz
>
> -! { dg-additional-options -Wuninitialized }
> +! { dg-additional-options "-Wuninitialized -O0" }
>
> 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 }
> + ! { 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
> - z = c
> + if (allocated(z)) z = c
> !$acc end parallel
> !$omp end target
> print *, z
-----------------
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
