[Bug middle-end/110639] New: [OpenMP][5.1] Predefined firstprivate for pointers - attachment missing

[Bug middle-end/110639] New: [OpenMP][5.1] Predefined firstprivate for pointers - attachment missing

[burnus at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110639

            Bug ID: 110639
           Summary: [OpenMP][5.1] Predefined firstprivate for pointers -
                    attachment missing
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Keywords: openmp, wrong-code
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: burnus at gcc dot gnu.org
  Target Milestone: ---

OpenMP 5.1 changed (quoting GCC impl. status):

Pointer predetermined firstprivate getting initialized to address of matching
mapped list item per 5.1, Sect. 2.21.7.2


However, looking at the examples in the OpenMP spec issue #1796 (TRAC864) this
does not handle partially mapped data correctly:

int a[100];
int *p = &a[0];

#pragma omp target teams distribute map(a[start:n])
for (int i = start; i < start+n; i++) {
  p[i] = f(p[i], i);
}

Here, 'p' has an points to 'a' (actually, to the base address of 'a') but this
address is not mapped.  However, it still points to the extended address range
of mapped data.

[Bug middle-end/110639] [OpenMP][5.1] Predefined firstprivate for pointers - attachment missing

[burnus at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110639

--- Comment #1 from Tobias Burnus <burnus at gcc dot gnu.org> ---
Testing shows that the offsets are correctly handled but that there is an
ordering problem. Example:

int
main ()
{
  int a[100] = {};
  int *p = &a[0];
  uintptr_t iptr;
  #pragma omp target map(a, iptr)
    iptr = (uintptr_t) p;

This will fail - as the implicitly added 'firstprivate' arrives too early at
GOMP_target_ext - before 'a' is mapped:

   map(alloc:MEM[(char *)p] [len: 0])
     map(firstprivate:p [pointer assign, bias: 0])
       map(tofrom:iptr [len: 8])
         map(tofrom:a [len: 400])

If 'a' is already present on the device (e.g. 'omp target enter data map(a)'),
it works.

Solution: The implicitly mapped C/C++ pointer variable 'p' must be added at the
end of the clauses.

[Bug middle-end/110639] [OpenMP][5.1] Predefined firstprivate for pointers - attachment missing

[burnus at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110639

--- Comment #2 from Tobias Burnus <burnus at gcc dot gnu.org> ---
> If 'a' is already present on the device (e.g. 'omp target enter data map(a)'), it works.

This applies to both the comment 0 example where only a section of 'a' is
mapped start > 0 and for the comment 1 example where the whole of 'a' is
mapped.

It also works fine if 'p' points inside 'A'.

* * *

As spec ref:

TR12 states in "14.8 target Construct" [379:8-10]:

"[C/C++] If a list item in a map clause has a base pointer that is
predetermined firstprivate (see Section 6.1.1) and on entry to the target
region the list item is mapped, the firstprivate pointer is updated via
corresponding base pointer initialization."


OpenMP 5.1 has in the mentioned C/C++-only section "2.21.7.2 Pointer
Initialization for Device Data Environments" that is too long to be quoted.


[The TR12 wording 'on entry to the target region' makes it clear that
effectively ordering needs to happen. The 5.1 wording is a bit unclear whether
it can be mapped with that very target construct - or the storage needs to be
present before the target directive. - However, the examples in OpenMP issue
#1796 implies that also 5.1 permit mapping the data and the pointer be on the
same directive.]

* * *

The implicit handling of the 'p' in this example happens in gimplify.cc's
gimplify_adjust_omp_clauses_1 for 'else if (code == OMP_CLAUSE_MAP && (flags &
GOVD_MAP_0LEN_ARRAY) != 0)'.

[Bug middle-end/110639] [OpenMP][5.1] Predefined firstprivate for pointers - attachment missing

[burnus at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110639

--- Comment #3 from Tobias Burnus <burnus at gcc dot gnu.org> ---
Created attachment 56804
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56804&action=edit
gimplify.cc patch to ensure that GOVD_MAP_0LEN_ARRAY comes last (does not fix
the issue)

I tried the attached patch see whether it fixes the problem. It doesn't as the
pointer-lookup-for-attachment seems to happen in an earlier 'for' loop than the
'for' loop that does the actual mapping for clauses on the same 'target'
directive (→ gomp_map_vars_internal).

Thus, either this patch is not required - or it is only required in addition;
in any case, it seems as if libgomp/target.c's gomp_map_vars_internal needs to
be modified.

[Bug middle-end/110639] [OpenMP][5.1] Predefined firstprivate for pointers - attachment missing

[burnus at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110639

--- Comment #4 from Tobias Burnus <burnus at gcc dot gnu.org> ---
There are *two* independent issues:

(A) Predefined firstprivate does not find mappings done in the same
    directive, e.g.

  int a[100];
  int *p = &a[0];
  #pragma omp target teams distribute map(a)
    p[0] = 5;


(B) The base pointer is not stored, hence, the following fails:

  int a[100];
  int *p = &a[0];
  //   #pragma omp target enter data map(a[10:])  /* same result */
  #pragma omp target teams distribute map(a[10:])
    p[15] = 5;

Here,
   map(a[10:])  /* or: map(a[start:n])  */
gives:
   map(tofrom:a[start] [len: _7])
      map(firstprivate:a [pointer assign, bias: D.2943])

But then the basepointer is gone. Thus, any later lookup of an address that
falls between basepointer and first mapped storage location is not found.

[Bug middle-end/110639] [OpenMP][5.1] Predefined firstprivate for pointers - attachment missing

[burnus at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110639

--- Comment #5 from Tobias Burnus <burnus at gcc dot gnu.org> ---
Posted a patch for (A)
  https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639947.html
but it seems as if I might have misunderstood some parts of the example at

  OpenMP spec issue #1796 (TRAC864) / OpenMP Pull Req. #912

Thus, this needs to be rechecked. - It might be that the current state of
mainline is just fine, that some parts of this patch still make sense, or that
more issues exist.