OpenMPTarget: fixes and workarounds to work with "Release" build type. #3748
Conversation
…d of Debug mode. Fixing bugs and implementing workarounds to work in the Release mode.
| @@ -217,41 +217,47 @@ template <class ExecutionSpace> | |||
| struct DeepCopy<Kokkos::Experimental::OpenMPTargetSpace, | |||
| Kokkos::Experimental::OpenMPTargetSpace, ExecutionSpace> { | |||
| DeepCopy(void* dst, const void* src, size_t n) { | |||
| omp_target_memcpy(dst, const_cast<void*>(src), n, 0, 0, | |||
| omp_get_default_device(), omp_get_default_device()); | |||
| if (n) | |||
There was a problem hiding this comment.
Readability
| if (n) | |
| if (n > 0) |
There was a problem hiding this comment.
I'd like you to add a comment in the code where you elaborate on why that call to omp_target_memcpy is protected. The specification does not explicitly calls out whether it is legal to pass null pointers with a zero length (see https://www.openmp.org/spec-html/5.1/openmpsu172.html). Did you check whether this was the issue?
Also, any good reason to ignore the return value here?
There was a problem hiding this comment.
Yes the specification does not say anything specific about passing zero length to omp_target_memcpy. I guess the Release build just protects against unnecessary copies.
I added a return value to the memcpy calls and am checking for its success.
…o builds of the OPENMPTARGET backend. Added a return value for the omp_target_memcpy.
|
The failing SYCL build is fixed by #3741. |
Throws exception on failure
dalg24
force-pushed
the
OpenMPTarget_fixes
branch
from
d462937
to
47700f0
Compare
| #ifdef KOKKOS_ENABLE_OPENMPTARGET | ||
| test.run(1, 32, 9); | ||
| test.run(2, 64, 22); | ||
| test.run(14, 128, 321); | ||
| #else |
There was a problem hiding this comment.
What is the reason for changing this?
There was a problem hiding this comment.
In the Release and RelWithDebInfo builds, the scratch implementation fails unless the team_size is a multiple of 32.
There was a problem hiding this comment.
Would you mind adding a FIXME with this comment (also in the other place)?
| #ifdef KOKKOS_ENABLE_OPENMPTARGET | ||
| test.run(14, 277, 1); | ||
| test.run(1, 32, 4); | ||
| test.run(4, 64, 10); | ||
| test.run(14, 128, 20); |
There was a problem hiding this comment.
Same as above, the team_size needs to be a multiple of 32 for Release builds.
rgayatri23
added
Compiler Issue
…elated to Release and RelWithDebInfo builds.
|
Retest this please. |
core/unit_test/TestReducers_d.hpp
Outdated
| // FIXME_OPENMPTARGET - The size of data in array_reduce has to be a power of | ||
| // 2 for OPENMPTARGET backend in Release and RelWithDebInfo builds. | ||
| #ifdef KOKKOS_ENABLE_OPENMPTARGET | ||
| TestReducers<array_reduce<float, 4>, TEST_EXECSPACE>::test_sum(1031); |
There was a problem hiding this comment.
this guy is now twice here isn't it?
Fixed it. |
Fixes
omp_target_memcpycalls are protected to be called only when the copy-size > 0 .nowaitclause to theTeamVectorRangeimplementation. Avoids hanging in Release builds and is also accurate for the corresponding Kokkos constructs.*reducers_size_t:*reducers_double:*reducers_struct. WIP to fix them.