Add allocV2 and freeV2 which return cl_mem on OpenCL backends by umar456 · Pull Request #2911 · arrayfire/arrayfire

umar456 · 2020-05-28T06:51:27Z

The af_alloc_device functions were returning C++ objects instead of C objects. This behavior is now decremented in favor of cl_mem objects in the OpenCL backends. The behavior of the CUDA and CPU backends have not changed.

Updated documentation to clarify proper usage and behavior

Deprecated af_alloc_device af_free_device

Added several memory tests.

docs/details/device.dox

include/af/memory.h

9prady9 · 2020-05-28T07:20:21Z

src/backend/opencl/memory.cpp


-void memLock(const void *ptr) {
-    memoryManager().userLock(const_cast<void *>(ptr));
+void memLock(const cl::Buffer *ptr) {


what is the road block for having something like the following:

use cl_mem everywhere starting from memory manager to internal backend API

only wrap them in cl::Buffer(mem_handle, true) locally where they are needed as cl::Buffers

I haven't analyzed all the pros and cons of above approach. was throwing it out there based on the point that hjaving cl_mem everyone in internal API will make it consistent

I think cl::Buffers are fine internally as long as the native functions are using cl_mem. The mem* functions are used internally and they should be returning cl::Buffers because that is most convenient for our code base. The reason I moved to the nativeAlloc interface is because the memory manager should be compatible with the base OpenCL specification. The C++ interface is not stable enough for that.

9prady9 · 2020-05-28T09:19:14Z

test/CMakeLists.txt

+      add_test(NAME ${target} COMMAND ${target})
+    endif()
+  endforeach()
+endif()


What am I missing here ? It feels almost identical to make_test macro.

cuda_add_executable(${target} cuda.cu)

Too much redundancy for single line difference :(

9prady9 · 2020-05-28T09:21:57Z

test/cuda.cu

+
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+TEST(Memory, AfAllocDeviceCUDA) {


Something like AllocDevicePointerUsability instead of AfAllocDeviceCUDA is more readable name.

its testing af_alloc_device. I think the name is appropriate here.

9prady9 · 2020-05-28T09:23:08Z

test/manual_memory_test.cpp

        }

-        ASSERT_EQ(true, false);  // Is there a simple assert statement?
+        FAIL();


9prady9 · 2020-05-28T09:32:44Z

test/memory.cpp

+    // This behavior will change in the future
+    ASSERT_EQ(payload->lastNdims, 1);
+    ASSERT_EQ(payload->lastDims, af::dim4(aSize * aSize * aSize * aSize));
+}


Tests: E2ETest*D and E2ETest4DComplexDouble can use a better name like MostRecentArrayCreated_*D or ``MostRecentArray_*D`

Most recent array created is not an appropriate name because it is only one of the many things it tests. in this case we are testing the behavior of the custom memory manager in the case we create a 4D array.

9prady9 · 2020-05-28T09:34:17Z

test/memory.cpp

+    ASSERT_EQ(payload->lastDims, af::dim4(aSize * aSize * aSize * aSize));
+}
+
+TEST_F(MemoryManagerApi, E2ETestMultipleAllocations) {


nit: Don't feel like the prefix E2ETest is adding anything to the readability factor..

9prady9 · 2020-05-28T09:36:28Z

test/memory.cpp

+
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+TEST(Memory, AfAllocDeviceCPUC) {


AfAllocDeviceCPUC what does C in the end mean ? for this and the following tests

Uses the C api instead of the C++ api

9prady9 · 2020-05-28T09:40:06Z

test/ocl_ext_context.cpp

 #endif
+
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"


We have to figure out a generic way (a custom drop in marco perhaps) to do this for gcc, clang and msvc if possible. Instead of adding compiler specific pragmas in multiple locations.

Not as a part of this PR, of course.

* Improve documentation of the alloc and free function * Add tests for memory operations

* Older alloc functions were returning cl::Buffer objects. This behavior is deprecated in favor of cl_mem objects on the OpenCL backend

Add out of memory test using custom memory manager

b2206ed

umar456 added OpenCL api-change labels May 28, 2020

umar456 modified the milestones: 3.7.2, 3.8.0 May 28, 2020

9prady9 requested changes May 28, 2020

View reviewed changes

umar456 force-pushed the mem branch 2 times, most recently from 3d54583 to f0b2664 Compare May 28, 2020 18:45

umar456 added 2 commits May 29, 2020 00:28

Return cl_mem instead of cl::Buffer from nativeAlloc

09aef8f

* Improve documentation of the alloc and free function * Add tests for memory operations

Create af::allocV2 and af::freeV2 which return cl_mem

761f8e5

* Older alloc functions were returning cl::Buffer objects. This behavior is deprecated in favor of cl_mem objects on the OpenCL backend

umar456 force-pushed the mem branch from f0b2664 to 761f8e5 Compare May 29, 2020 04:29

umar456 requested a review from 9prady9 May 29, 2020 04:52

9prady9 approved these changes May 29, 2020

View reviewed changes

9prady9 merged commit f620f76 into arrayfire:master May 29, 2020

umar456 deleted the mem branch May 30, 2020 18:49

umar456 mentioned this pull request May 31, 2020

Memory manager OOM unit test #2897

Closed

Conversation

umar456 commented May 28, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants