Re: [PATCH v3 06/11] OpenMP: Pointers and member mappings

[Tobias Burnus <tobias@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 9779 bytes --]

Hi Julian and Jakub, hi all,

On 23.09.22 09:29, Julian Brown wrote:

How about this version? (Re-tested.)

[...]

* * *

Some more generic (pre)remarks – not affecting the patch code,
but possibly the commit log message:


This follows OMP 5.0, 2.19.7.1 "map Clause":

which is also in "OMP 5.2, 5.8.3 map Clause [152:1-4]". It might
make sense to add this ref in addition (or instead):


  "If a list item in a map clause is an associated pointer and the
   pointer is not the base pointer of another list item in a map clause
   on the same construct, then it is treated as if its pointer target
   is implicitly mapped in the same clause. For the purposes of the map
   clause, the mapped pointer target is treated as if its base pointer
   is the associated pointer."


Pre-remark 1: Issue with the wording in the 5.x spec. Namely, assume

integer, pointer :: p(:)
! allocate(p(1024)); deallocate(p) ! < (un)commented lined or not

!$omp target map(p)
  p => null()
!$omp end target

Here, 'p' is neither associated nor unassociated (NULL pointer),
but it is undefined. However, the 5.x spec does require an implicit mapping of
the associated pointer target – but the compiler has no idea whether
the pointer address is valid (associated) or dangling (undefined) - and
the p.data address might be either invalid to access and/or the p.dim[...]
garbage data could yield a size(p) that is huge.


Thus, the following restriction was proposed for OpenMP 6.0 (TR11):

"The association status of a list item that is a pointer must not be
 undefined unless it is a structure component and it results from a
 predefined default mapper."

which makes my example invalid. (Add some caveat here about TR11 not
yet being released and also TRs being not final named-version releases.)

This also affects the quote you show, which now reads (2nd line is new):

"If a list item in a map clause is an associated pointer
 that is not attach-ineligible
 and the pointer is not the base pointer [...]".

with 'attach-ineligible' being defined in the previous bullet list
(i.e. in the preceding paragraph (>= 5.1) about derived-type components;
first bullet point there: pointer implies attach-ineligible).

I think the 5.x wording has issues, but on the other hand, the wording
above is not in an OpenMP release and not even in a TR. As the spirit
has not changed, it probably makes sense to keep the 5.0 (5.x) wording.

Cf. https://github.com/OpenMP/spec/pull/3319/files and
https://github.com/OpenMP/spec/issues/3290
(And apologies to non-OpenMP members as those are non-publicly accessible.)

 * * *



However, we can also write this:
  map(to: tvar%arrptr) map(tofrom: tvar%arrptr(3:8))

and then instead we should follow:

  "If the structure sibling list item is a pointer then it is treated
   as if its association status is undefined, unless it appears as
   the base pointer of another list item in a map clause on the same
   construct."

This wording disappeared in 5.1 due to some cleanup (cf. Issue 2152, which has multiple changes; this one is Pull Req. 2379). I think the matching current / OpenMP 5.2 wording (5.8.3 map Clause [152:5-8, 11-13 (,14-16)]) is "For map clauses on map-entering constructs, if any list item has a base pointer for which a corresponding pointer exists in the data environment upon entry to the region and either a new list item or the corresponding pointer is created in the device data environment on entry to the region, then: (Fortran) 1. The corresponding pointer variable is associated with a pointer target that has the same rank and bounds as the pointer target of the original pointer, such that the corresponding list item can be accessed through the pointer in a target region. ..." I think here 'a new list item ... is created ... on entry' applies. However, this should not affect what you wrote later on. Still, I wonder whether 5.2 instead of (I think it makes sense to also read 5.2 when implementing this for bug-fix changes (and trivial feature changes), but not for larger-effort changes, which can be implemented later on.)

But, that's not implemented quite right at the moment [...]
The solution is to detect when we're mapping a smaller part of the array
(or a subcomponent) on the same directive, and only map the descriptor
in that case. So we get mappings like this instead:

  map(to: tvar%arrptr)   -->
  GOMP_MAP_ALLOC  tvar%arrptr  (the descriptor)

  map(tofrom: tvar%arrptr(3:8)   -->
  GOMP_MAP_TOFROM tvar%arrptr%data(3) (size 8-3+1, etc.)
  GOMP_MAP_ALWAYS_POINTER tvar%arrptr%data (bias 3, etc.)

(I concur.)

* * *

--- a/gcc/fortran/trans-openmp.cc
+++ b/gcc/fortran/trans-openmp.cc
...
@@ -2470,22 +2471,18 @@ gfc_trans_omp_array_section (stmtblock_t *block, gfc_omp_namelist *n,
     }
   if (GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (decl)))
     {
...
+      if (ptr_kind != GOMP_MAP_ALWAYS_POINTER)
        {
...
+         /* For OpenMP, the descriptor must be mapped with its own explicit
+            map clause (e.g. both "map(foo%arr)" and "map(foo%arr(:))" must
+            be present in the clause list if "foo%arr" is a pointer to an
+            array).  So, we don't create a GOMP_MAP_TO_PSET node here.  */
+         node2 = build_omp_clause (input_location, OMP_CLAUSE_MAP);
+         OMP_CLAUSE_SET_MAP_KIND (node2, GOMP_MAP_TO_PSET);

I found the last sentence of the comment and the set_map_kind confusing:
The comment says no MAP_TO_PSET and the SET_MAP_KIND use it.

I wonder whether that should be something like 'if (openacc)' instead,
which kind of matches the first way gfc_trans_omp_array_section is called:

                  else if (lastref->type == REF_ARRAY)
                    {
                      /* An array element or section.  */
                      bool element = lastref->u.ar.type == AR_ELEMENT;
                      gomp_map_kind kind = (openacc ? GOMP_MAP_ATTACH_DETACH
                                                    : GOMP_MAP_ALWAYS_POINTER);
                      gfc_trans_omp_array_section (block, n, inner, element,
                                                   kind, node, node2, node3,
                                                   node4);
However, there is also a second call to it:

                  /* An array element or array section which is not part of a
                     derived type, etc.  */
...
                  gomp_map_kind k = GOMP_MAP_POINTER;
...
                  gfc_trans_omp_array_section (block, n, decl, element, k,
                                               node, node2, node3, node4);

And without following all 'if' conditions through, I don't see why that should
be handled differently.


+  /* We're only interested in cases where we have an expression, e.g. a
+     component access.  */
+  if (n->expr && n->expr->symtree)
+    use_sym = n->expr->symtree->n.sym;

The second part looks unsafe in light of 'lvalues'. The next OpenMP version
will likely permit:
"A locator list item is a variable list item, a function reference with data
pointer result, or a reserved locator."

Thus, permitting 'map( f(cnt=4))' for a function that returns a data pointer like
  interface
    function f(cnt) result(res)
      integer :: cnt
      real, pointer :: res(:,:)
    end
  end interface

(In Fortran, referencing 'f' counts as variable. However, contrary to C/C++,
'f()%comp or 'f()(1:4)', i.e. component and array reverences, are not permitted.)

Thus, it seems to make more sense to have n->expr->expr_type == EXPR_VARIABLE
as the symtree is also set for EXPR_FUNCTION and EXPR_COMPCALL.
The later is something like  dt%f(5)  where 'dt' is a variable of derived type
and 'f' is a variable bound to the derived type. – I think it might also be
set for PARAMETER, but I am not sure until which point.



+         if (!n2->expr || !n2->expr->symtree)
+           continue;

Likewise.

+                         /* If the last reference is a pointer to a derived
+                            type ("foo%dt_ptr"), check if any subcomponents
+                            of the same derived type member are being mapped
+                            elsewhere in the clause list ("foo%dt_ptr%x",
+                            etc.).  If we have such subcomponent mappings,
+                            we only create an ALLOC node for the pointer
+                            itself, and inhibit mapping the whole derived
+                            type.  */

Does the current code handle also the following?

  i = 1; j = 2
  map (foo(i)%dt_ptr(1:3), foo(j)%dt_ptr)

Note: I have not thought about validity nor checked your code, but it does
not seem to be completely odd code to write.

A similar mean way to write code would be:

integer, target :: A(5)
integer, pointer :: p(:), p2(:)
type(t) :: var

allocate(p2(1:20))
p => A
var%p2 => p2
!$omp target map(A(3:4), p2(4:8), p, var%p2)
 ....
!$omp end target

which has a similar issue – it is not clear from the syntax whether
p's or var%p2's pointer target has been mapped or not.

I don't currently see whether that's handled or not - but I fear it is not.
All this seems to points to first handle all non-pointer variables,
then all with tailing array refs and only then the rest - and implicit mapping
last (followed by use_device_{ptr,addr}).

Tobias


-----------------
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