Hi Julian, Tobias! 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. > 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
