Passing size==0 to DeepCopy/memcpy/omp_target_memcpy #6273

nliber · 2023-07-10T22:45:27Z

This is to address issue #6034.

This approach is more general in that no work happens with DeepCopy, memcpy or omp_target_memcpy when a zero size is passed to it. This avoids the issue of reading an invalid pointer value.

nliber · 2023-07-11T18:19:50Z

retest this please

crtrott · 2023-07-14T21:39:28Z

Retest this please.

masterleinad

Please add a test.

core/src/HIP/Kokkos_HIP_Instance.cpp

core/src/impl/Kokkos_Profiling.cpp

core/src/impl/Kokkos_HostSpace_deepcopy.cpp

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Parallel_Common.hpp

0-length ranges, as per this comment in core/src/OpenMPTarget/Kokkos_OpenMPTargetSpace.hpp // In the Release and RelWithDebInfo builds, the size of the memcpy should // be greater than zero to avoid error.

Removed unnecessary size check in Kokkos_Profiling.cpp Fixed size check in Kokkos_HostSpace_deepcopy.cpp Fixed size check in Kokkos_OpenMPTarget_Parallel_Common.hpp

Issues reported were addressed

crtrott · 2023-08-01T22:37:26Z

@masterleinad Is there anything missing in this test:

kokkos/core/unit_test/TestViewCopy_b.hpp

Lines 205 to 239 in 1a3ea28

    
           TEST(TEST_CATEGORY, view_copy_degenerated) { 
        
             Kokkos::View<int*, TEST_EXECSPACE, Kokkos::MemoryTraits<Kokkos::Unmanaged>> 
        
                 v_um_def_1; 
        
             Kokkos::View<int*, TEST_EXECSPACE, Kokkos::MemoryTraits<Kokkos::Unmanaged>> 
        
             v_um_1(reinterpret_cast<int*>(-1), 0); 
        
             Kokkos::View<int*, TEST_EXECSPACE> v_m_def_1; 
        
             Kokkos::View<int*, TEST_EXECSPACE> v_m_1("v_m_1", 0); 
        
             Kokkos::View<int*, TEST_EXECSPACE, Kokkos::MemoryTraits<Kokkos::Unmanaged>> 
        
                 v_um_def_2; 
        
             Kokkos::View<int*, TEST_EXECSPACE, Kokkos::MemoryTraits<Kokkos::Unmanaged>> 
        
             v_um_2(reinterpret_cast<int*>(-1), 0); 
        
             Kokkos::View<int*, TEST_EXECSPACE> v_m_def_2; 
        
             Kokkos::View<int*, TEST_EXECSPACE> v_m_2("v_m_2", 0); 
        
             Kokkos::deep_copy(v_um_def_1, v_um_def_2); 
        
             Kokkos::deep_copy(v_um_def_1, v_um_2); 
        
             Kokkos::deep_copy(v_um_def_1, v_m_def_2); 
        
             Kokkos::deep_copy(v_um_def_1, v_m_2); 
        
             Kokkos::deep_copy(v_um_1, v_um_def_2); 
        
             Kokkos::deep_copy(v_um_1, v_um_2); 
        
             Kokkos::deep_copy(v_um_1, v_m_def_2); 
        
             Kokkos::deep_copy(v_um_1, v_m_2); 
        
             Kokkos::deep_copy(v_m_def_1, v_um_def_2); 
        
             Kokkos::deep_copy(v_m_def_1, v_um_2); 
        
             Kokkos::deep_copy(v_m_def_1, v_m_def_2); 
        
             Kokkos::deep_copy(v_m_def_1, v_m_2); 
        
             Kokkos::deep_copy(v_m_1, v_um_def_2); 
        
             Kokkos::deep_copy(v_m_1, v_um_2); 
        
             Kokkos::deep_copy(v_m_1, v_m_def_2); 
        
             Kokkos::deep_copy(v_m_1, v_m_2); 
        
           }

crtrott · 2023-08-01T22:37:51Z

Retest this please

masterleinad

Looks OK to me.

crtrott · 2023-08-02T03:18:36Z

Retest this please.

crtrott · 2023-08-02T03:19:03Z

Looks like OpenMPTarget didn't run, I want to see it at least compile since a bunch of OpenMPTarget specific files were touched.

Rombur · 2023-08-02T12:55:21Z

Retest this please

fnrizzi · 2023-08-03T11:46:31Z

only OpenMPTarget fails, error is:

5: Test command: /var/jenkins/workspace/Kokkos/build/simd/unit_tests/Kokkos_UnitTest_SIMD
35: Test timeout computed to be: 1500
35: [==========] Running 2 tests from 1 test suite.
35: [----------] Global test environment set-up.
35: [----------] 2 tests from simd
35: [ RUN      ] simd.host
35: [       OK ] simd.host (0 ms)
35: [ RUN      ] simd.device
35: CUDA error: an illegal memory access was encountered 
35: Libomptarget error: Call to targetDataEnd failed, abort target.
35: Libomptarget error: Failed to process data after launching the kernel.
35: Libomptarget error: Consult https://openmp.llvm.org/design/Runtimes.html for debugging options.
35: Kokkos_OpenMPTarget_ParallelFor_Range.hpp:54:1: Libomptarget fatal error 1: failure of target construct while offloading is mandatory
35/35 Test #35: Kokkos_UnitTest_SIMD ........................Child aborted***Exception: 114.34 sec

97% tests passed, 1 tests failed out of 35

rgayatri23 · 2023-08-03T16:38:05Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Parallel_Common.hpp

@@ -33,9 +33,12 @@ struct ParallelReduceCopy {
  static void memcpy_result(PointerType dest, PointerType src, size_t size,
                            bool ptr_on_device) {
    if (ptr_on_device) {


Just to mention, everywhere else you have

if(0<size) if(ptr_on_device)

While here you have flipped the order of the if conditions. Should not make a difference but just wanted to mention.

rgayatri23 · 2023-08-03T17:00:18Z

The test passed on a A100 machine.

Retest this please.

crtrott · 2023-08-04T03:20:29Z

OpenMPTarget passed on retest. I am assuming that this is an intermittent failure NOT caused by this PR.

nliber changed the base branch from master to develop July 10, 2023 22:45

nliber marked this pull request as draft July 10, 2023 22:45

nliber mentioned this pull request Jul 10, 2023

Passing nullptr to unmanaged views #6034

Closed

nliber marked this pull request as ready for review July 12, 2023 22:23

crtrott approved these changes Jul 14, 2023

View reviewed changes

masterleinad requested changes Jul 14, 2023

View reviewed changes

dalg24 previously requested changes Jul 14, 2023

View reviewed changes

nliber added 4 commits July 21, 2023 20:29

In deep_copy(...), don't call Impl::DeepCopy when nbytes == 0

b5cc79c

Don't pass 0-length ranges to std::memcpy

74346cf

Added protections around omp_target_memcpy so as not to pass

ac60f71

0-length ranges, as per this comment in core/src/OpenMPTarget/Kokkos_OpenMPTargetSpace.hpp // In the Release and RelWithDebInfo builds, the size of the memcpy should // be greater than zero to avoid error.

Removed unnecessary size check in Kokkos_HIP_Instance.cpp

ae15db7

Removed unnecessary size check in Kokkos_Profiling.cpp Fixed size check in Kokkos_HostSpace_deepcopy.cpp Fixed size check in Kokkos_OpenMPTarget_Parallel_Common.hpp

nliber force-pushed the issue6034 branch from 466ccdf to ae15db7 Compare July 21, 2023 21:24

masterleinad approved these changes Aug 1, 2023

View reviewed changes

rgayatri23 reviewed Aug 3, 2023

View reviewed changes

crtrott merged commit f557d3d into kokkos:develop Aug 4, 2023
27 of 28 checks passed

masterleinad mentioned this pull request Sep 14, 2023

CHANGELOG: 4.2.0 #6197

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Passing size==0 to DeepCopy/memcpy/omp_target_memcpy #6273

Passing size==0 to DeepCopy/memcpy/omp_target_memcpy #6273

nliber commented Jul 10, 2023

nliber commented Jul 11, 2023

crtrott commented Jul 14, 2023

masterleinad left a comment

crtrott commented Aug 1, 2023

crtrott commented Aug 1, 2023

masterleinad left a comment

crtrott commented Aug 2, 2023

crtrott commented Aug 2, 2023

Rombur commented Aug 2, 2023

fnrizzi commented Aug 3, 2023

rgayatri23 Aug 3, 2023

rgayatri23 commented Aug 3, 2023

crtrott commented Aug 4, 2023

Passing size==0 to DeepCopy/memcpy/omp_target_memcpy #6273

Passing size==0 to DeepCopy/memcpy/omp_target_memcpy #6273

Conversation

nliber commented Jul 10, 2023

nliber commented Jul 11, 2023

crtrott commented Jul 14, 2023

masterleinad left a comment

Choose a reason for hiding this comment

crtrott commented Aug 1, 2023

crtrott commented Aug 1, 2023

masterleinad left a comment

Choose a reason for hiding this comment

crtrott commented Aug 2, 2023

crtrott commented Aug 2, 2023

Rombur commented Aug 2, 2023

fnrizzi commented Aug 3, 2023

rgayatri23 Aug 3, 2023

Choose a reason for hiding this comment

rgayatri23 commented Aug 3, 2023

crtrott commented Aug 4, 2023