FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

  1. arrayfire
  2. arrayfire

Repository navigation

Add allocV2 and freeV2 which return cl_mem on OpenCL backends - #2911

Merged
9prady9 merged 3 commits into
arrayfire:masterfrom
umar456:mem
May 29, 2020
ConversationCommits (3)ChecksFiles changed
Merged

Add allocV2 and freeV2 which return cl_mem on OpenCL backends#2911
9prady9 merged 3 commits into
arrayfire:masterfrom
umar456:mem

Conversation

umar456 commented May 28, 2020

Copy link
Copy Markdown
Member

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.

umar456 modified the milestones: 3.7.2, 3.8.0 May 28, 2020
9prady9 requested changes May 28, 2020
View reviewed changes
Comment thread docs/details/device.dox Outdated
Comment thread include/af/memory.h
Comment thread src/backend/opencl/memory.cpp

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

9prady9 May 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

umar456 May 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread test/CMakeLists.txt
add_test(NAME ${target} COMMAND ${target})
endif()
endforeach()
endif()

9prady9 May 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

umar456 May 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

cuda_add_executable(${target} cuda.cu)

9prady9 May 29, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Too much redundancy for single line difference :(

Comment thread test/cuda.cu

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

9prady9 May 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Something like AllocDevicePointerUsability instead of AfAllocDeviceCUDA is more readable name.

umar456 May 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread test/manual_memory_test.cpp
}

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

9prady9 May 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

👍

Comment thread 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));
}

9prady9 May 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

umar456 May 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread test/memory.cpp
ASSERT_EQ(payload->lastDims, af::dim4(aSize * aSize * aSize * aSize));
}

TEST_F(MemoryManagerApi, E2ETestMultipleAllocations) {

9prady9 May 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread test/memory.cpp

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

9prady9 May 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

umar456 May 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Uses the C api instead of the C++ api

Comment thread test/ocl_ext_context.cpp
#endif

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

9prady9 May 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

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 requested a review from 9prady9 May 29, 2020 04:52
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
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

9prady9 9prady9 approved these changes

Assignees

No one assigned

Labels

api-change OpenCL

Projects

None yet

Milestone

3.8.0

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL