| Message ID | 4fc42e9c-676d-f3fc-57ab-f665a679bfbb@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fortran: Fix Array dependency with local coarrays [PR98913] (was: Re: A question about coarrays and -Warray-temporaries) | expand |
Hi Tobias,, ----- Mensaje original ----- > De: "Tobias Burnus" > Para: "Thomas Koenig" , "jdelia" <jdelia@cimec.unl.edu.ar>, "Gfortran List" > gcc.gnu.org>, "GCC Patches" <gcc-patches@gcc.gnu.org> > Enviados: Martes, 2 de Febrero 2021 8:46:00 > Asunto: [Patch] Fortran: Fix Array dependency with local coarrays [PR98913] (was: Re: A question about coarrays and > -Warray-temporaries) > > Hi all, > > the attached patch now handles -fcoarray=single and access to the local > image like no coarray access, using the same check a before. I would like to apply the patch but, sorry, how do I proceed? Where in the gcc tree to apply the patch file? Should I use git apply or diff? e.g. it does not work with $ pwd /home/bigpack/gcc-paq/sources/gcc-11.0-20210202 $ git apply dep-caf.diff error: patch failed: gcc/fortran/dependency.c:30 error: gcc/fortran/dependency.c: patch does not apply > Actually, I think we could remove the if (..coarray..) check completely: > For 'single', it is like no coarrays; for 'lib' both AA[remote] =... and > "AA = ... AA[remove]" will create a function call and uses already > temporaries. I don't know what 'native'/'threads' does – but either it > the image index is different, then no temporary is needed at all – or it > is the same (in particular: this_image / not present), but then the > noncoarray analysis would work. > > (Knowing that for i != this_image(), AA and AA[i] – or i /= j for AA[i] > and AA[j] – we could further relax the checks, but I don't think that > that's needed.) > > Hence: Is the patch OK for the trunk? Or should we completely get rid of > if-conditional block? Thanks for the fast work with this code optimization issue. Regards, Jorge. > -- > On 01.02.21 12:52, Tobias Burnus wrote: > >> On 01.02.21 08:42, Thomas Koenig via Fortran wrote: >>>> I have a question with -Warray-temporaries in the test below. >>>> With the AA coarray that warning appears in the first loop (on its >>>> local part), >>>> but not with the BB array with the same task, i.e. [...] >>> It seems that dependency checking does not detect that no overlap >>> can exist in that case, and so generates a temporary. Probably, >>> dependency checking was not updated when coarrays were introduced. >>> This is a missed optimization, not a correctness issue. >> I have now filled https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98913 > ----------------- > Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München > Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank > Thürauf
Am 02.02.21 um 12:46 schrieb Tobias Burnus: > Hi all, > > the attached patch now handles -fcoarray=single and access to the local > image like no coarray access, using the same check a before. > > Actually, I think we could remove the if (..coarray..) check completely: > For 'single', it is like no coarrays; for 'lib' both AA[remote] =... and > "AA = ... AA[remove]" will create a function call and uses already > temporaries. I don't know what 'native'/'threads' does – but either it > the image index is different, then no temporary is needed at all – or it > is the same (in particular: this_image / not present), but then the > noncoarray analysis would work. That analysis is correct, also as far as the shared memory coarray goes (where you do offsets as an extra dimension, either it points to the same memory or to different memory). So, while your patch is OK, I think a simple removal of the test is also OK. Take your pick :-) Best regards Thomas
Hi Thomas, On 02.02.21 15:54, Thomas Koenig wrote: > So, while your patch is OK, I think a simple removal of the test > is also OK. > Take your pick :-) I think I will do a combination: If 'identical' is true, I think I cannot remove it. If it is false, it can be identical or nonoverlapping – which makes sense. Unless there are further comments, I will commit it later. Tobias ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf
Fortran: Fix Array dependency with local coarrays [PR98913] gcc/fortran/ChangeLog: PR fortran/98913 * dependency.c (gfc_dep_resolver): Treat local access to coarrays like any array access in dependency analysis. gcc/testsuite/ChangeLog: PR fortran/98913 * gfortran.dg/coarray/array_temporary.f90: New test. gcc/fortran/dependency.c | 11 +++- .../gfortran.dg/coarray/array_temporary.f90 | 74 ++++++++++++++++++++++ 2 files changed, 83 insertions(+), 2 deletions(-) diff --git a/gcc/fortran/dependency.c b/gcc/fortran/dependency.c index c9baca80cbc..0de5d093aab 100644 --- a/gcc/fortran/dependency.c +++ b/gcc/fortran/dependency.c @@ -30,6 +30,7 @@ along with GCC; see the file COPYING3. If not see #include "dependency.h" #include "constructor.h" #include "arith.h" +#include "options.h" /* static declarations */ /* Enums */ @@ -2143,8 +2144,14 @@ gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gfc_reverse *reverse, case REF_ARRAY: - /* For now, treat all coarrays as dangerous. */ - if (lref->u.ar.codimen || rref->u.ar.codimen) + /* For now, treat all nonlocal coarrays as dangerous. */ + if (flag_coarray != GFC_FCOARRAY_SINGLE + && ((lref->u.ar.codimen + && lref->u.ar.dimen_type[lref->u.ar.dimen] + != DIMEN_THIS_IMAGE) + || (rref->u.ar.codimen + && lref->u.ar.dimen_type[lref->u.ar.dimen] + != DIMEN_THIS_IMAGE))) return 1; if (ref_same_as_full_array (lref, rref)) diff --git a/gcc/testsuite/gfortran.dg/coarray/array_temporary.f90 b/gcc/testsuite/gfortran.dg/coarray/array_temporary.f90 new file mode 100644 index 00000000000..86460a7c282 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/coarray/array_temporary.f90 @@ -0,0 +1,74 @@ +! { dg-do compile } +! { dg-additional-options "-Warray-temporaries" } +! +! PR fortran/98913 +! +! Contributed by Jorge D'Elia +! +! Did create an array temporary for local access to coarray +! (but not for identical noncoarray use). +! + +program test + implicit none + integer, parameter :: iin = kind (1) + integer, parameter :: idp = kind (1.0d0) + real (kind=idp), allocatable :: AA (:,:)[:] + real (kind=idp), allocatable :: BB (:,:) + real (kind=idp), allocatable :: UU (:) + integer (kind=iin) :: nn, n1, n2 + integer (kind=iin) :: j, k, k1 + ! + nn = 5 + n1 = 1 + n2 = 10 + ! + allocate (AA (1:nn,n1:n2)[*]) + allocate (BB (1:nn,n1:n2)) + allocate (UU (1:nn)) + ! + k = 1 + k1 = k + 1 + ! + AA = 1.0_idp + BB = 1.0_idp + UU = 2.0_idp + + ! AA - coarrays + ! No temporary needed: + do j = 1, nn + AA (k1:nn,j) = AA (k1:nn,j) - UU (k1:nn) * AA (k,j) ! { dg-bogus "Creating array temporary" } + end do + do j = 1, nn + AA (k1:nn,j) = AA (k1:nn,j) - UU (k1:nn) * AA (k,j) - UU(k) * AA (k1-1:nn-1,j) ! { dg-bogus "Creating array temporary" } + end do + do j = 1, nn + AA (k1:nn,j) = AA (k1:nn,j) - UU (k1:nn) * AA (k,j) - UU(k) * AA (k1+1:nn+1,j) ! { dg-bogus "Creating array temporary" } + end do + + ! But: + do j = 1, nn + AA (k1:nn,j) = AA (k1-1:nn-1,j) - UU (k1:nn) * AA (k,j) - UU(k) * AA (k1+1:nn+1,j) ! { dg-warning "Creating array temporary" } + end do + + ! BB - no coarrays + ! No temporary needed: + do j = 1, nn + BB (k1:nn,j) = BB (k1:nn,j) - UU (k1:nn) * BB (k,j) ! { dg-bogus "Creating array temporary" } + end do + do j = 1, nn + BB (k1:nn,j) = BB (k1:nn,j) - UU (k1:nn) * BB (k,j) - UU(k) * BB (k1-1:nn-1,j) ! { dg-bogus "Creating array temporary" } + end do + do j = 1, nn + BB (k1:nn,j) = BB (k1:nn,j) - UU (k1:nn) * BB (k,j) - UU(k) * BB (k1+1:nn+1,j) ! { dg-bogus "Creating array temporary" } + end do + + ! But: + do j = 1, nn + BB (k1:nn,j) = BB (k1-1:nn-1,j) - UU (k1:nn) * BB (k,j) - UU(k) * BB (k1+1:nn+1,j) ! { dg-warning "Creating array temporary" } + end do + + deallocate (AA) + deallocate (BB) + deallocate (UU) +end program test