-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[OpenMp] Prototype OpenMP 5.1's omp_target_is_accessible #143058
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
These changes originate from the clacc project Original author: Joel E. Denny Original commit: 198fde1 I require the omp_target_is_accessible functionality for current development, so I'm submitting Joel E. Denny's implementation for community review. This contribution: 1. Implements OpenMP 5.1's omp_target_is_accessible interface 2. Introduces device memory accessibility verification logic 3. Requests technical validation for: - Edge case handling (Size=0 scenarios) - NULL pointer behavior specification - Compatibility with shared memory architectures Please help verify implementation correctness against OpenMP specifications.
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
@llvm/pr-subscribers-offload Author: None (yuanm-qy) ChangesThese changes originate from the clacc project I require the omp_target_is_accessible functionality for current development, so I'm submitting Joel E. Denny's implementation for community review. This contribution:
Please help verify implementation correctness against OpenMP specifications. Full diff: https://github.com/llvm/llvm-project/pull/143058.diff 4 Files Affected:
diff --git a/offload/include/omptarget.h b/offload/include/omptarget.h
index 6971780c7bdb5..8af8c4f659b35 100644
--- a/offload/include/omptarget.h
+++ b/offload/include/omptarget.h
@@ -280,6 +280,7 @@ int omp_get_initial_device(void);
void *omp_target_alloc(size_t Size, int DeviceNum);
void omp_target_free(void *DevicePtr, int DeviceNum);
int omp_target_is_present(const void *Ptr, int DeviceNum);
+int omp_target_is_accessible(const void *Ptr, size_t Size, int DeviceNum);
int omp_target_memcpy(void *Dst, const void *Src, size_t Length,
size_t DstOffset, size_t SrcOffset, int DstDevice,
int SrcDevice);
diff --git a/offload/libomptarget/OpenMP/API.cpp b/offload/libomptarget/OpenMP/API.cpp
index 4576f9bd06121..936c366e7e3a3 100644
--- a/offload/libomptarget/OpenMP/API.cpp
+++ b/offload/libomptarget/OpenMP/API.cpp
@@ -683,3 +683,60 @@ EXTERN void *omp_get_mapped_ptr(const void *Ptr, int DeviceNum) {
return TPR.TargetPointer;
}
+
+EXTERN int omp_target_is_accessible(const void *Ptr, size_t Size,
+ int DeviceNum) {
+ TIMESCOPE();
+ // OpenMP 5.1, sec. 3.8.4 "omp_target_is_accessible", p. 417, L21-22:
+ // "This routine returns true if the storage of size bytes starting at the
+ // address given by Ptr is accessible from device device_num. Otherwise, it
+ // returns false."
+ //
+ // The meaning of "accessible" for unified shared memory is established in
+ // OpenMP 5.1, sec. 2.5.1 "requires directive". More generally, the specified
+ // host memory is accessible if it can be accessed from the device either
+ // directly (because of unified shared memory or because DeviceNum is the
+ // value returned by omp_get_initial_device()) or indirectly (because it's
+ // mapped to the device).
+ DP("Call to omp_target_is_accessible for device %d and address " DPxMOD "\n",
+ DeviceNum, DPxPTR(Ptr));
+
+ // FIXME: Is this right?
+ //
+ // Null pointer is permitted:
+ //
+ // OpenMP 5.1, sec. 3.8.4 "omp_target_is_accessible", p. 417, L15:
+ // "The value of ptr must be a valid host pointer or NULL (or C_NULL_PTR, for
+ // Fortran)."
+ //
+ // However, I found no specification of behavior in this case.
+ // omp_target_is_present has the same problem and is implemented the same way.
+ // Should Size have any effect on the result when Ptr is NULL?
+ if (!Ptr) {
+ DP("Call to omp_target_is_accessible with NULL Ptr, returning false\n");
+ return false;
+ }
+
+ if (DeviceNum == omp_get_initial_device()) {
+ DP("Call to omp_target_is_accessible on host, returning true\n");
+ return true;
+ }
+
+ auto DeviceOrErr = PM->getDevice(DeviceNum);
+ if (!DeviceOrErr)
+ FATAL_MESSAGE(DeviceNum, "%s", toString(DeviceOrErr.takeError()).c_str());
+
+ // TODO: How does the spec intend for the Size=0 case to be handled?
+ // Currently, for the case where arr[N:M] is mapped, we return true for any
+ // address within arr[0:N+M]. However, Size>1 returns true only for arr[N:M].
+ // This is based on the discussion so far at the time of this writing at
+ // <https://github.com/llvm/llvm-project/issues/54899>. If the behavior
+ // changes, keep comments for omp_get_accessible_buffer in omp.h.var in sync.
+ TargetPointerResultTy TPR =
+ DeviceOrErr->getMappingInfo().getTgtPtrBegin(const_cast<void *>(Ptr), Size,
+ /*UpdateRefCount=*/false,
+ /*UseHoldRefCount=*/false);
+ int Rc = TPR.isPresent();
+ DP("Call to omp_target_is_accessible returns %d\n", Rc);
+ return Rc;
+}
\ No newline at end of file
diff --git a/offload/libomptarget/exports b/offload/libomptarget/exports
index 2406776c1fb5f..236efd263f7ce 100644
--- a/offload/libomptarget/exports
+++ b/offload/libomptarget/exports
@@ -43,6 +43,7 @@ VERS1.0 {
omp_target_alloc;
omp_target_free;
omp_target_is_present;
+ omp_target_is_accessible;
omp_target_memcpy;
omp_target_memcpy_rect;
omp_target_memcpy_async;
diff --git a/openmp/runtime/src/kmp_ftn_os.h b/openmp/runtime/src/kmp_ftn_os.h
index ae0ed067235e5..46afa800379e0 100644
--- a/openmp/runtime/src/kmp_ftn_os.h
+++ b/openmp/runtime/src/kmp_ftn_os.h
@@ -114,6 +114,7 @@
#define FTN_TARGET_ALLOC omp_target_alloc
#define FTN_TARGET_FREE omp_target_free
#define FTN_TARGET_IS_PRESENT omp_target_is_present
+#define FTN_TARGET_IS_ACCESSIBLE omp_target_is_accessible
#define FTN_TARGET_MEMCPY omp_target_memcpy
#define FTN_TARGET_MEMCPY_RECT omp_target_memcpy_rect
#define FTN_TARGET_MEMSET omp_target_memset
@@ -263,6 +264,7 @@
#define FTN_TARGET_ALLOC omp_target_alloc_
#define FTN_TARGET_FREE omp_target_free_
#define FTN_TARGET_IS_PRESENT omp_target_is_present_
+#define FTN_TARGET_IS_ACCESSIBLE omp_target_is_accessible_
#define FTN_TARGET_MEMCPY omp_target_memcpy_
#define FTN_TARGET_MEMCPY_RECT omp_target_memcpy_rect_
#define FTN_TARGET_ASSOCIATE_PTR omp_target_associate_ptr_
@@ -412,6 +414,7 @@
#define FTN_TARGET_ALLOC OMP_TARGET_ALLOC
#define FTN_TARGET_FREE OMP_TARGET_FREE
#define FTN_TARGET_IS_PRESENT OMP_TARGET_IS_PRESENT
+#define FTN_TARGET_IS_ACCESSIBLE OMP_TARGET_IS_ACCESSIBLE
#define FTN_TARGET_MEMCPY OMP_TARGET_MEMCPY
#define FTN_TARGET_MEMCPY_RECT OMP_TARGET_MEMCPY_RECT
#define FTN_TARGET_ASSOCIATE_PTR OMP_TARGET_ASSOCIATE_PTR
@@ -559,6 +562,7 @@
#define FTN_TARGET_ALLOC OMP_TARGET_ALLOC_
#define FTN_TARGET_FREE OMP_TARGET_FREE_
#define FTN_TARGET_IS_PRESENT OMP_TARGET_IS_PRESENT_
+#define FTN_TARGET_IS_ACCESSIBLE OMP_TARGET_IS_ACCESSIBLE_
#define FTN_TARGET_MEMCPY OMP_TARGET_MEMCPY_
#define FTN_TARGET_MEMCPY_RECT OMP_TARGET_MEMCPY_RECT_
#define FTN_TARGET_ASSOCIATE_PTR OMP_TARGET_ASSOCIATE_PTR_
|
OpenACC? OpenMP? |
The implementation of the function does not match what OpenMP specifies. The current implementation looks like a copy of Instead, the function should check, whether the memory region can be directly accessed by means of unified memory |
These changes originate from the clacc project
Original author: Joel E. Denny
Original commit: 198fde1
I require the omp_target_is_accessible functionality for current development, so I'm submitting Joel E. Denny's implementation for community review. This contribution:
Please help verify implementation correctness against OpenMP specifications.