-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Add cmake for majel #2118
Add cmake for majel #2118
Changes from all commits
b0f070e
3a73455
9c501e9
92e0c89
d8c8f41
05b3196
d444251
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,14 @@ add_subdirectory(pserver) | |
add_subdirectory(trainer) | ||
add_subdirectory(scripts) | ||
|
||
find_package(boost QUIET) | ||
|
||
if(Boost_FOUND) | ||
include_directories(${Boost_INCLUDE_DIRS}) | ||
include_directories(${CMAKE_CURRENT_SOURCE_DIR}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 我对cmake不是很熟悉,为何这里要把当前目录给include了? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explained in #2122 |
||
add_subdirectory(majel) | ||
endif() | ||
|
||
if(WITH_C_API) | ||
add_subdirectory(capi) | ||
endif() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
build | ||
third-party |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
cmake_minimum_required(VERSION 3.0) | ||
|
||
if(GTEST_INCLUDE_DIR AND GTEST_LIBRARIES) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that we are building Docker container, we can assume in CMakeLists.txt that we have boost and gtest already installed and don't need conditions like this, isn't it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explained in #2122 |
||
message("-- Found gtest (include: ${GTEST_INCLUDE_DIR}, library: ${GTEST_LIBRARIES})") | ||
else() | ||
# find #include <majel/xx.h> | ||
get_filename_component(PARENT_DIR ${CMAKE_CURRENT_SOURCE_DIR} DIRECTORY) | ||
include_directories(${PARENT_DIR}) | ||
|
||
# find cmake directory modules | ||
get_filename_component(PARENT_DIR ${PARENT_DIR} DIRECTORY) | ||
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${PARENT_DIR}/cmake") | ||
|
||
# enable c++11 | ||
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11") | ||
|
||
# enable gtest | ||
set(THIRD_PARTY_PATH ${CMAKE_CURRENT_SOURCE_DIR}/third_party) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. third_party要不要放到运行cmake的目录(比如 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explained in #2122 |
||
set(WITH_TESTING ON) | ||
include(external/gtest) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain where does "external" directory come from (have not found any directory named "external" after building majel)? This line works, but I don't understand how it works... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explained in #2122 |
||
endif() | ||
|
||
########################### Build Majel ############################# | ||
set(MAJEL_CXX_FILES place.cpp) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PADDLE includes both CPU and GPU versions. If CMake detects the host machine equipped GPUs, it will invoke NOTE: The suffix of unit test files should strictly limit to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If we are going to port Majel to Paddle, I think we will have to enable GPU and CPU versions. Also, we'll have to be able to specify which one to build, as we do with Paddle. Also, I think we shouldn't assume that the development computer having a GPU means that the runtime environment would have GPUs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree that if a source file doesn't include CUDA code, the suffix shouldn't be .cu. But I personally prefer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explained in #2122 |
||
set(MAJEL_CUDA_FILES "") | ||
|
||
if(CUDA_FOUND) | ||
cuda_add_library(majel ${MAJEL_CUDA_FILES} ${MAJEL_CXX_FILES}) | ||
else() | ||
add_library(majel ${MAJEL_CXX_FILES}) | ||
endif() | ||
##################################################################### | ||
|
||
add_subdirectory(test) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
#include <majel/place.h> | ||
|
||
namespace majel { | ||
|
||
namespace detail { | ||
|
||
class PlacePrinter : public boost::static_visitor<> { | ||
private: | ||
std::ostream& os_; | ||
|
||
public: | ||
PlacePrinter(std::ostream& os) : os_(os) {} | ||
|
||
void operator()(const CpuPlace&) { os_ << "CpuPlace"; } | ||
|
||
void operator()(const GpuPlace& p) { os_ << "GpuPlace(" << p.device << ")"; } | ||
}; | ||
|
||
} // namespace majel | ||
|
||
static Place the_default_place; | ||
|
||
void set_place(const Place& place) { the_default_place = place; } | ||
|
||
const Place& get_place() { return the_default_place; } | ||
|
||
const GpuPlace default_gpu() { return GpuPlace(0); } | ||
|
||
const CpuPlace default_cpu() { return CpuPlace(); } | ||
|
||
bool is_gpu_place(const Place& p) { | ||
return boost::apply_visitor(IsGpuPlace(), p); | ||
} | ||
|
||
bool is_cpu_place(const Place& p) { | ||
return !boost::apply_visitor(IsGpuPlace(), p); | ||
} | ||
|
||
bool places_are_same_class(const Place& p1, const Place& p2) { | ||
return is_gpu_place(p1) == is_gpu_place(p2); | ||
} | ||
|
||
std::ostream& operator<<(std::ostream& os, const majel::Place& p) { | ||
majel::detail::PlacePrinter printer(os); | ||
boost::apply_visitor(printer, p); | ||
return os; | ||
} | ||
|
||
} // namespace majel |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
file(GLOB_RECURSE ALL_TEST_FILES RELATIVE "${CMAKE_CURRENT_SOURCE_DIR}" "*.cpp" "*.cc") | ||
|
||
add_executable(majel_tests ${ALL_TEST_FILES}) | ||
add_dependencies(majel_tests majel) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 请问为何既然有了
还需要
感觉不是重复地指定依赖了? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explained in #2122 |
||
target_link_libraries(majel_tests | ||
${Boost_LIBRARIES} | ||
${GTEST_LIBRARIES} | ||
majel | ||
) | ||
add_test(majel_tests majel_tests) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
#include "gtest/gtest.h" | ||
#include "majel/place.h" | ||
#include <sstream> | ||
#include "gtest/gtest.h" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry that I forgot to reorder the include files following https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes. You can do that in this PR. I think the right order is: #include "majel/place.h"
#include <sstream>
#include "gtest/gtest.h" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explained in #2122 |
||
|
||
TEST(Place, Equality) { | ||
majel::CpuPlace cpu; | ||
|
@@ -18,12 +18,12 @@ TEST(Place, Equality) { | |
} | ||
|
||
TEST(Place, Default) { | ||
EXPECT_TRUE(majel::is_gpu_place( majel::get_place())); | ||
EXPECT_TRUE(majel::is_gpu_place( majel::default_gpu())); | ||
EXPECT_TRUE(majel::is_cpu_place( majel::default_cpu())); | ||
EXPECT_TRUE(majel::is_gpu_place(majel::get_place())); | ||
EXPECT_TRUE(majel::is_gpu_place(majel::default_gpu())); | ||
EXPECT_TRUE(majel::is_cpu_place(majel::default_cpu())); | ||
|
||
majel::set_place(majel::CpuPlace()); | ||
EXPECT_TRUE(majel::is_cpu_place( majel::get_place())); | ||
EXPECT_TRUE(majel::is_cpu_place(majel::get_place())); | ||
} | ||
|
||
TEST(Place, Print) { | ||
|
@@ -33,8 +33,8 @@ TEST(Place, Print) { | |
EXPECT_EQ("GpuPlace(1)", ss.str()); | ||
} | ||
{ | ||
std::stringstream ss; | ||
ss << majel::CpuPlace(); | ||
EXPECT_EQ("CpuPlace", ss.str()); | ||
std::stringstream ss; | ||
ss << majel::CpuPlace(); | ||
EXPECT_EQ("CpuPlace", ss.str()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
#include "gtest/gtest.h" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why we need this file? I think we can just link There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explained in #2122 |
||
|
||
int main(int argc, char** argv) { | ||
::testing::InitGoogleTest(&argc, argv); | ||
return RUN_ALL_TESTS(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我记得design doc里说用轻量级的boost替代品,这里仍然是用的boost吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里是boost,不是替代品。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explained in #2122