[Bug libgomp/109837] New: [OpenMP] despite 'requires unified_address' there is segfault when 'is_device_ptr' is not used

[Bug libgomp/109837] New: [OpenMP] despite 'requires unified_address' there is segfault when 'is_device_ptr' is not used

[burnus at gcc dot gnu.org]

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

            Bug ID: 109837
           Summary: [OpenMP] despite 'requires unified_address' there is
                    segfault when 'is_device_ptr' is not used
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: libgomp
          Assignee: unassigned at gcc dot gnu.org
          Reporter: burnus at gcc dot gnu.org
                CC: jakub at gcc dot gnu.org
  Target Milestone: ---

Cf. https://github.com/SOLLVE/sollve_vv/pull/734 for a testcase.

For nvptx, we currently have:

GOMP_OFFLOAD_get_num_devices (unsigned int omp_requires_mask)
...
      && ((omp_requires_mask
           & ~(GOMP_REQUIRES_UNIFIED_ADDRESS
               | GOMP_REQUIRES_REVERSE_OFFLOAD)) != 0))

That is: we accept for nvptx

  omp requires unfied_address

However, while the address space is the same, the following is not handled:

> the is_device_ptr clause is not necessary to obtain device addresses from device pointers for use inside target regions.

Expected:
(A) Mapping related: is_device_ptr can be left out.
(B) omp_target_is_accessible - will properly work for such pointers.


For nvptx, the check can be done via cudaPointerGetAttributes
if I understand
https://docs.nvidia.com/cuda/cuda-runtime-api/group__CUDART__UNIFIED.html
correctly.

 * * *

NOTE: Something similar is needed for GCN, except that its
GOMP_OFFLOAD_get_num_devices currently returns -1 when
GOMP_REQUIRES_UNIFIED_ADDRESS has been requested.

It seems as if hsa_amd_pointer_info is the function to be used,
cf.
https://github.com/RadeonOpenCompute/ROCR-Runtime/blob/master/src/inc/hsa_ext_amd.h

[Bug libgomp/109837] [OpenMP] despite 'requires unified_address' there is segfault when 'is_device_ptr' is not used

[burnus at gcc dot gnu.org]

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

--- Comment #1 from Tobias Burnus <burnus at gcc dot gnu.org> ---
Minor correction/clarification:

  (B) omp_target_is_accessible - will properly work for such pointers.

While it might be useful to handle also (C/C++) code like:
  ptr = omp_target_alloc (...);
  ...
  is_accessible = omp_target_is_accessible(ptr, ...);

The OpenMP spec only requires:
"The *omp_target_is_accessible* routine tests whether host memory is accessible
from a given device."

And "ptr" is a device pointer. Thus, returning FALSE for all such pointers,
unless in shared memory, is OK.

→ Still, we might consider to add checking for memory that is both host and
device accessible - or is a device pointer in the unified-address case.
(Quality of Implementation Feature).

* * *

Besides 'is_device_ptr', the same is required for has_device_addr like in
(Fortran):

  c_ptr = omp_alloc(...)
  call c_f_pointer (c_ptr, fptr)
  !$omp target
    fptr = ...

[That has_device_addr is not mentioned for 'requires unified_addr' is the
OpenMP Spec Issue #3606.]

[Bug libgomp/109837] [OpenMP] despite 'requires unified_address' there is segfault when 'is_device_ptr' is not used

[burnus at gcc dot gnu.org]

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

--- Comment #2 from Tobias Burnus <burnus at gcc dot gnu.org> ---
Actually, I missed a crucial fine print regarding 'defaultmap(default)'
implicit mapping of pointers in C/C++. GCC actually works (with firstprivate or
with normal mapping)
→ https://github.com/SOLLVE/sollve_vv/pull/737

TODO: It does not harm to have some Fortran ('type(c_ptr)') and C/C++ testcase
in libgomp/testsuite/ but nothing else needs to be done.

(For GCN, enabling unified_address makes sense.)

[Bug libgomp/109837] [OpenMP] despite 'requires unified_address' there is segfault when 'is_device_ptr' is not used

[burnus at gcc dot gnu.org]

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

--- Comment #3 from Tobias Burnus <burnus at gcc dot gnu.org> ---
(In reply to Tobias Burnus from comment #2)
> (For GCN, enabling unified_address makes sense.)

Now done via commit r14-1584-gf1af7d65ff64fe7102d1490ef46ea491a533e641

> TODO: It does not harm to have some Fortran ('type(c_ptr)') and C/C++
> testcase in libgomp/testsuite/ but nothing else needs to be done.

Still to be done.

[Bug libgomp/109837] [OpenMP] despite 'requires unified_address' there is segfault when 'is_device_ptr' is not used

[cvs-commit at gcc dot gnu.org]

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

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tobias Burnus <burnus@gcc.gnu.org>:

https://gcc.gnu.org/g:d5c58ad1ebaff924c2546df074174cffb128feb8

commit r14-1783-gd5c58ad1ebaff924c2546df074174cffb128feb8
Author: Tobias Burnus <tobias@codesourcery.com>
Date:   Tue Jun 13 11:27:47 2023 +0200

    libgomp/testsuite: Add requires-unified-addr-1.{c,f90} [PR109837]

    Add a testcase for 'omp requires unified_address' that is currently
supported
    by all devices but was not tested for.

    libgomp/

            PR libgomp/109837
            * testsuite/libgomp.c-c++-common/requires-unified-addr-1.c: New
test.
            * testsuite/libgomp.fortran/requires-unified-addr-1.f90: New test.

[Bug libgomp/109837] [OpenMP] despite 'requires unified_address' there is segfault when 'is_device_ptr' is not used

[burnus at gcc dot gnu.org]

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

Tobias Burnus <burnus at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #5 from Tobias Burnus <burnus at gcc dot gnu.org> ---
FIXED.

Namely:
* Code handling was correct, once the testcase was fix ...
  → See sollve_vv link in comment 0 and esp. in comment 2.

* For GCN, unified_address support had to be enabled
  → trivial patch, see comment 3

* We did not have a testcase in libgomp
  → commit in comment 4

=> All done → Close as FIXED.