Skip to content
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

enable operator gpu unittest #3050

Merged
merged 19 commits into from
Aug 2, 2017
Merged

Conversation

QiJune
Copy link
Member

@QiJune QiJune commented Jul 25, 2017

No description provided.

@QiJune QiJune force-pushed the op_gpu_test branch 2 times, most recently from 2aae619 to 4a1f7bd Compare July 31, 2017 09:42
@QiJune QiJune changed the title [WIP]enable operator gpu unittest enable operator gpu unittest Aug 1, 2017
@@ -1,3 +1,4 @@
#define EIGEN_USE_GPU
Copy link
Member Author

@QiJune QiJune Aug 1, 2017

Choose a reason for hiding this comment

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

Have to add this macro, otherwise, will cause segment fault

std::memcpy(dst, array.data(), sizeof(T) * array.size());
}

#ifndef PADDLE_ONLY_CPU
template <typename T>
void PyCUDATensorSetFromArray(
Copy link
Member Author

Choose a reason for hiding this comment

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

partial specialization of function template is not allowed. So just define another function here

places = []
places.append(core.CPUPlace())
if core.is_compile_gpu():
places.append(core.GPUPlace(0))
Copy link
Member Author

@QiJune QiJune Aug 1, 2017

Choose a reason for hiding this comment

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

Run CPU OpKernel first, and then GPU OpKernel

@@ -8,8 +8,8 @@ class TestAddOp(unittest.TestCase):

def setUp(self):
self.type = "add_two"
self.X = numpy.random.random((342, 345)).astype("float32")
self.Y = numpy.random.random((342, 345)).astype("float32")
self.X = numpy.random.random((102, 105)).astype("float32")
Copy link
Member Author

Choose a reason for hiding this comment

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

Sometimes, GPU memory is not enough for unit-test, so reduce the size

# TODO(qijun) gcc 4.9 or later versions raise SEGV due to the optimization problem.
# Use Debug mode instead for now.
if(CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 4.9 OR CMAKE_CXX_COMPILER_VERSION VERSION_EQUAL 4.9)
set(CMAKE_BUILD_TYPE "Debug" CACHE STRING "" FORCE)
Copy link
Member Author

Choose a reason for hiding this comment

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

If gcc 4.8 and nvcc 8.0, both debug and release will be fine.
If gcc 5.4 and nvcc 8.0, debug is fine, but release will cause segment fault.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about we constraint GCC version in Dockerfile, other than checking here?

I know checking in CMake provides a guarantee, but I am afraid adding too many such constraints would complicate our building system too much.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I will constraint GCC version in Dockerfile.
But some others may compile paddle in gcc5.4 environment, so, the unittest test_framework will fall.

@@ -62,9 +61,11 @@ inline T* Tensor::mutable_data(platform::Place place) {
if (platform::is_cpu_place(place)) {
holder_.reset(new PlaceholderImpl<T, platform::CPUPlace>(
boost::get<platform::CPUPlace>(place), size));
} else if (platform::is_gpu_place(place)) {
#ifdef PADDLE_ONLY_CPU
PADDLE_THROW("'GPUPlace' is not supported in CPU only device.");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

remove }

Copy link
Member Author

Choose a reason for hiding this comment

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

{ is outside of macro, so } is needed inside macro

Copy link
Contributor

@gangliao gangliao left a comment

Choose a reason for hiding this comment

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

LGTM

@QiJune QiJune merged commit 6824c09 into PaddlePaddle:develop Aug 2, 2017
@QiJune QiJune moved this from Doing to Done in PaddlePaddle Refactoring: Phase 1 Aug 3, 2017
heavengate pushed a commit to heavengate/Paddle that referenced this pull request Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants