Hi Julian, Tobias! On 2020-07-14T13:43:37+0200, I wrote: > On 2020-06-16T15:39:44-0700, Julian Brown <jul...@codesourcery.com> wrote: >> As mentioned in the blurb for the previous patch, an "attach" operation >> for a Fortran pointer with an array descriptor must copy that array >> descriptor to the target. > > Heh, I see -- I don't think I had read the OpenACC standard in that way, > but I think I agree your interpretation is fine. > > This does not create some sort of memory leak -- everything implicitly > allocated there will eventually be deallocated again, right? > >> This patch arranges for that to be so. > > In response to the new OpenACC/Fortran testcase that I'd submtited in > <87wo3co0tm.fsf@euler.schwinge.homeip.net">http://mid.mail-archive.com/87wo3co0tm.fsf@euler.schwinge.homeip.net>, > you (Julian) correctly supposed in > <http://mid.mail-archive.com/20200709223246.23a4d0e0@squid.athome>, that > this patch indeed does resolve that testcase, too. That wasn't obvious > to me. So, similar to > 'libgomp/testsuite/libgomp.oacc-c-c++-common/pr95270-{1.2}.c', please > include my new OpenACC/Fortran testcase (if that makes sense to you), and > reference PR95270 in the commit log.
My new OpenACC/Fortran testcase got again broken ('libgomp: pointer target not mapped for attach') by Tobias' commit 102502e32ea4e8a75d6b252ba319d09d735d9aa7 "[OpenMP, Fortran] Add structure/derived-type element mapping", <c5b43e02-d1d5-e7cf-c11c-6daf1e8f33c5@codesourcery.com">http://mid.mail-archive.com/c5b43e02-d1d5-e7cf-c11c-6daf1e8f33c5@codesourcery.com>. Similar ('libgomp: attempt to attach null pointer') for your new 'libgomp.oacc-fortran/attach-descriptor-1.f90'. (Whether or not 'attach'ing 'NULL' should actually be allowed, is a separate topic for discussion.) So this patch here will (obviously) need to be adapted to what Tobias changed. (Plus my more general questions quoted above and below.) Grüße Thomas >> OK? > > Basically yes (for master and releases/gcc-10 branches), but please > consider the following: > >> --- a/gcc/fortran/trans-openmp.c >> +++ b/gcc/fortran/trans-openmp.c >> @@ -2573,8 +2573,44 @@ gfc_trans_omp_clauses (stmtblock_t *block, >> gfc_omp_clauses *clauses, >> } >> } >> if (GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (decl)) >> - && n->u.map_op != OMP_MAP_ATTACH >> - && n->u.map_op != OMP_MAP_DETACH) >> + && (n->u.map_op == OMP_MAP_ATTACH >> + || n->u.map_op == OMP_MAP_DETACH)) >> + { >> + tree type = TREE_TYPE (decl); >> + tree data = gfc_conv_descriptor_data_get (decl); >> + if (present) >> + data = gfc_build_cond_assign_expr (block, present, >> + data, >> + null_pointer_node); >> + tree ptr >> + = fold_convert (build_pointer_type (char_type_node), >> + data); >> + ptr = build_fold_indirect_ref (ptr); >> + /* Standalone attach clauses used with arrays with >> + descriptors must copy the descriptor to the target, >> + else they won't have anything to perform the >> + attachment onto (see OpenACC 2.6, "2.6.3. Data >> + Structures with Pointers"). */ >> + OMP_CLAUSE_DECL (node) = ptr; >> + node2 = build_omp_clause (input_location, OMP_CLAUSE_MAP); >> + OMP_CLAUSE_SET_MAP_KIND (node2, GOMP_MAP_TO_PSET); >> + OMP_CLAUSE_DECL (node2) = decl; >> + OMP_CLAUSE_SIZE (node2) = TYPE_SIZE_UNIT (type); >> + node3 = build_omp_clause (input_location, OMP_CLAUSE_MAP); >> + if (n->u.map_op == OMP_MAP_ATTACH) >> + { >> + OMP_CLAUSE_SET_MAP_KIND (node3, GOMP_MAP_ATTACH); >> + n->u.map_op = OMP_MAP_ALLOC; >> + } >> + else /* OMP_MAP_DETACH. */ >> + { >> + OMP_CLAUSE_SET_MAP_KIND (node3, GOMP_MAP_DETACH); >> + n->u.map_op = OMP_MAP_RELEASE; >> + } >> + OMP_CLAUSE_DECL (node3) = data; >> + OMP_CLAUSE_SIZE (node3) = size_int (0); >> + } > > So this ("case A") duplicates most of the code from... > >> + else if (GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (decl))) >> { >> [...] > > ... this existing case here ("case B"). It's not clear to me if these > two cases really still need to be handled separately, and a little bit > differently (regarding 'if (present)' handling, for example), or if they > could/should (?) be merged? Tobias, do you have an opinion? > > Do we have sufficient testsuite coverage? (For example, > 'attach'/'detach' with 'present == false', if that makes sense, or any > other thing that case A is doing differently from case B?) Shouldn't > this get '-fdump-tree-original' and/or '-fdump-tree-gimple' testcases, > similar to 'gfortran.dg/goacc/finalize-1.f', so that we verify/document > what we generate here? > > >> --- /dev/null >> +++ b/libgomp/testsuite/libgomp.oacc-fortran/attach-descriptor-1.f90 >> @@ -0,0 +1,51 @@ >> +program att > > Please add 'dg-do run', and... > >> + use openacc >> + implicit none >> + type t >> + integer :: arr1(10) >> + integer, allocatable :: arr2(:) >> + end type t >> + integer :: i >> + type(t) :: myvar >> + integer, target :: tarr(10) >> + integer, pointer :: myptr(:) >> + >> + allocate(myvar%arr2(10)) >> + >> + do i=1,10 >> + myvar%arr1(i) = 0 >> + myvar%arr2(i) = 0 >> + tarr(i) = 0 >> + end do >> + >> + call acc_copyin(myvar) >> + call acc_copyin(myvar%arr2) >> + call acc_copyin(tarr) >> + >> + myptr => tarr >> + >> + !$acc enter data attach(myvar%arr2, myptr) >> + >> + ! FIXME: This warning is emitted on the wrong line number. >> + ! { dg-warning "using vector_length \\(32\\), ignoring 1" "" { target >> openacc_nvidia_accel_selected } 36 } > > ... don't forget to adjust "36" here. ;-) > >> + !$acc serial present(myvar%arr2) >> + do i=1,10 >> + myvar%arr1(i) = i >> + myvar%arr2(i) = i >> + end do >> + myptr(3) = 99 >> + !$acc end serial >> + >> + !$acc exit data detach(myvar%arr2, myptr) >> + >> + call acc_copyout(myvar%arr2) >> + call acc_copyout(myvar) >> + call acc_copyout(tarr) >> + >> + do i=1,10 >> + if (myvar%arr1(i) .ne. i) stop 1 >> + if (myvar%arr2(i) .ne. i) stop 2 >> + end do >> + if (tarr(3) .ne. 99) stop 3 >> + >> +end program att > > > Grüße > Thomas ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter