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

Reply via email to