Skip to content

Move paddle/majel/* to paddle/{platform,framework/* #2513

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jun 20, 2017
+356 −356
Merged

Move paddle/majel/* to paddle/{platform,framework/* #2513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bb88202
Move paddle/majel/* to paddle/platform/ and paddle/framework/
Jun 19, 2017
9b5fb27
Make place_test buildable and passed.
Jun 19, 2017
d03159b
Use Google Style in new directories
Jun 19, 2017
8913aff
Make paddle/framework buildable and passed
Jun 19, 2017
b984ed9
Remove unused majle/.gitignore
Jun 19, 2017
8e49033
Fix Google Style
Jun 20, 2017
3124b9d
Make Google style include order
Jun 20, 2017
599b57a
Check boost for new code, as it uses boost::variant
wangkuiyi Jun 20, 2017
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions paddle/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ add_subdirectory(strings)

find_package(Boost QUIET)


if(Boost_FOUND)
Copy link
Contributor

Choose a reason for hiding this comment

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

if(Boost_FOUND)
include_directories(${Boost_INCLUDE_DIRS})
add_subdirectory(platform)
add_subdirectory(framework)
endif()

We need this line to guarantee the compiler can find the header file.
include_directories(${Boost_INCLUDE_DIRS})
include_directories(${CMAKE_CURRENT_SOURCE_DIR})
add_subdirectory(majel)
add_subdirectory(platform)
add_subdirectory(framework)
endif()

if(WITH_C_API)
Expand Down
5 changes: 5 additions & 0 deletions paddle/framework/.clang-format
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
Language: Cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

why we need this new clang-format file? We only have one in Paddle/.clang-format.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we also use cpplint as a complementary ? Because it already in cmake/clint.cmake, maybe we can invokes the wrapper function add_style_check_target in generic functions.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

/.clang-format was created by @reyoung to fit existing code style. However, we'd like to follow strict Google C++ style, so that during code review we could paste links into Google style's online doucment to help use explain pros and cons of programming choices.

clang-format works by looking for .clang-format in the nearest parent directory of the source file. So, as we put the new .clang-format in paddle/framework, clang-format would check paddle/framework/ddim.h and other source files following Google C++ style.
Copy link
Collaborator Author

@wangkuiyi wangkuiyi Jun 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I am only enforcing Google C++ style for new directories. Do you mean we call cpplint to check new code? I'd love to have it, but it seems cannot reformat the code like clang-format. Do we really need it?
Copy link
Contributor

@gangliao gangliao Jun 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure, just remind here.

It will scan [static analysis] and trigger the error if your code is "out of the box", for instance, if you did not add // namespace platform comment at the end, aha, here is a buggy.
BasedOnStyle: Google
Standard: Cpp11
...
4 changes: 4 additions & 0 deletions paddle/framework/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
cc_library(ddim SRCS ddim.cc)
cc_test(ddim_test SRCS ddim_test.cc DEPS ddim)

nv_test(dim_test SRCS dim_test.cu DEPS ddim)
18 changes: 10 additions & 8 deletions paddle/majel/ddim.cc → paddle/framework/ddim.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "paddle/majel/ddim.h"
#include "paddle/framework/ddim.h"

namespace majel {
namespace paddle {
namespace framework {

///@cond HIDDEN

Expand Down Expand Up @@ -66,28 +67,28 @@ DDim make_ddim(const std::vector<int>& dims) {
///@cond HIDDEN
// XXX For some reason, putting this in an anonymous namespace causes errors
class DynamicMutableIndexer : public boost::static_visitor<int&> {
public:
public:
DynamicMutableIndexer(int idx) : idx_(idx) {}

template <int D>
int& operator()(Dim<D>& dim) const {
return dim[idx_];
}

private:
private:
int idx_;
};

class DynamicConstIndexer : public boost::static_visitor<int> {
public:
public:
DynamicConstIndexer(int idx) : idx_(idx) {}

template <int D>
int operator()(const Dim<D>& dim) const {
return dim[idx_];
}

private:
private:
int idx_;
};

Expand Down Expand Up @@ -213,10 +214,11 @@ struct DDimPrinter : boost::static_visitor<void> {

///\endcond

std::ostream& operator<<(std::ostream& os, const majel::DDim& ddim) {
std::ostream& operator<<(std::ostream& os, const DDim& ddim) {
DDimPrinter printer(os);
boost::apply_visitor(printer, ddim);
return os;
}

} // namespace majel
} // namespace framework
} // namespace paddle
23 changes: 9 additions & 14 deletions paddle/majel/ddim.h → paddle/framework/ddim.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,14 @@
#include <stdexcept>
#include <vector>

#include "paddle/majel/dim.h"
#include "paddle/framework/dim.h"

namespace majel {
namespace paddle {
namespace framework {

namespace {
typedef boost::variant<Dim<1>,
Dim<2>,
Dim<3>,
Dim<4>,
Dim<5>,
Dim<6>,
Dim<7>,
Dim<8>,
Dim<9>>
typedef boost::variant<Dim<1>, Dim<2>, Dim<3>, Dim<4>, Dim<5>, Dim<6>, Dim<7>,
Dim<8>, Dim<9>>
DDimVar;
}

Expand Down Expand Up @@ -95,14 +89,15 @@ ssize_t product(const DDim& ddim);

int arity(const DDim& ddim);

std::ostream& operator<<(std::ostream&, const majel::DDim&);
std::ostream& operator<<(std::ostream&, const DDim&);

} // namespace majel
} // namespace framework
} // namespace paddle

namespace boost {

template <typename T>
T get(const majel::DDim& in) {
T get(const paddle::framework::DDim& in) {
return boost::get<T>(in.var);
}

Expand Down
26 changes: 13 additions & 13 deletions paddle/majel/ddim_test.cc → paddle/framework/ddim_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,62 +4,62 @@
#include <vector>

#include "gtest/gtest.h"
#include "paddle/majel/ddim.h"
#include "paddle/framework/ddim.h"

TEST(DDim, Equality) {
// construct a DDim from an initialization list
majel::DDim ddim = majel::make_ddim({9, 1, 5});
paddle::framework::DDim ddim = paddle::framework::make_ddim({9, 1, 5});
EXPECT_EQ(ddim[0], 9);
EXPECT_EQ(ddim[1], 1);
EXPECT_EQ(ddim[2], 5);

// construct a DDim from a vector
std::vector<int> vec({9, 1, 5});
majel::DDim vddim = majel::make_ddim(vec);
paddle::framework::DDim vddim = paddle::framework::make_ddim(vec);
EXPECT_EQ(ddim[0], 9);
EXPECT_EQ(ddim[1], 1);
EXPECT_EQ(ddim[2], 5);

// mutate a DDim
ddim[1] = 2;
EXPECT_EQ(ddim[1], 2);
majel::set(ddim, 0, 6);
EXPECT_EQ(majel::get(ddim, 0), 6);
paddle::framework::set(ddim, 0, 6);
EXPECT_EQ(paddle::framework::get(ddim, 0), 6);

// vectorize a DDim
std::vector<int> res_vec = majel::vectorize(vddim);
std::vector<int> res_vec = paddle::framework::vectorize(vddim);
EXPECT_EQ(res_vec[0], 9);
EXPECT_EQ(res_vec[1], 1);
EXPECT_EQ(res_vec[2], 5);
majel::Dim<3> d(3, 2, 1);
res_vec = majel::vectorize(majel::DDim(d));
paddle::framework::Dim<3> d(3, 2, 1);
res_vec = paddle::framework::vectorize(paddle::framework::DDim(d));
EXPECT_EQ(res_vec[0], 3);
EXPECT_EQ(res_vec[1], 2);
EXPECT_EQ(res_vec[2], 1);

// add two DDims
majel::DDim ddim_sum = ddim + vddim;
paddle::framework::DDim ddim_sum = ddim + vddim;
EXPECT_EQ(ddim_sum[0], 15);
EXPECT_EQ(ddim_sum[1], 3);
EXPECT_EQ(ddim_sum[2], 10);

// multiply two DDims
majel::DDim ddim_mul = ddim * vddim;
paddle::framework::DDim ddim_mul = ddim * vddim;
EXPECT_EQ(ddim_mul[0], 54);
EXPECT_EQ(ddim_mul[1], 2);
EXPECT_EQ(ddim_mul[2], 25);

// arity of a DDim
EXPECT_EQ(majel::arity(ddim), 3);
EXPECT_EQ(paddle::framework::arity(ddim), 3);

// product of a DDim
EXPECT_EQ(majel::product(vddim), 45);
EXPECT_EQ(paddle::framework::product(vddim), 45);
}

TEST(DDim, Print) {
// print a DDim
std::stringstream ss;
majel::DDim ddim = majel::make_ddim({2, 3, 4});
paddle::framework::DDim ddim = paddle::framework::make_ddim({2, 3, 4});
ss << ddim;
EXPECT_EQ("2, 3, 4", ss.str());
}
24 changes: 13 additions & 11 deletions paddle/majel/dim.h → paddle/framework/dim.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@
#include <stdexcept>
#include <type_traits>

#include "paddle/majel/detail/cuda_assert.h"
#include "paddle/majel/detail/hostdevice.h"
#include "paddle/platform/assert.h"
#include "paddle/platform/hostdevice.h"

namespace majel {
namespace paddle {
namespace framework {

// Statically sized, statically indexed dimension
template <int i>
Expand Down Expand Up @@ -74,7 +75,7 @@ struct Dim<1> {
throw std::invalid_argument("Index out of range.");
}
#else
MAJEL_ASSERT(idx < size.head);
PADDLE_ASSERT(idx < size.head);
#endif
}

Expand Down Expand Up @@ -131,7 +132,7 @@ HOSTDEVICE int& indexer(Dim<D>& dim, int idx) {
throw std::invalid_argument("Tried to access a negative dimension");
}
#else
MAJEL_ASSERT(idx >= 0);
PADDLE_ASSERT(idx >= 0);
#endif
if (idx == 0) {
return dim.head;
Expand All @@ -146,7 +147,7 @@ HOSTDEVICE int& indexer<1>(Dim<1>& dim, int idx) {
throw std::invalid_argument("Invalid index");
}
#else
MAJEL_ASSERT(idx == 0);
PADDLE_ASSERT(idx == 0);
#endif
return dim.head;
}
Expand All @@ -158,7 +159,7 @@ HOSTDEVICE int indexer(const Dim<D>& dim, int idx) {
throw std::invalid_argument("Tried to access a negative dimension");
}
#else
MAJEL_ASSERT(idx >= 0);
PADDLE_ASSERT(idx >= 0);
#endif
if (idx == 0) {
return dim.head;
Expand All @@ -173,7 +174,7 @@ HOSTDEVICE int indexer<1>(const Dim<1>& dim, int idx) {
throw std::invalid_argument("Invalid index");
}
#else
MAJEL_ASSERT(idx == 0);
PADDLE_ASSERT(idx == 0);
#endif
return dim.head;
}
Expand Down Expand Up @@ -411,7 +412,7 @@ HOSTDEVICE Dim<sizeof...(Args)> make_dim(Args... idxes) {
// XXX For some reason, overloading fails to resolve this correctly
template <int i>
typename std::enable_if<(i > 1), std::ostream&>::type operator<<(
std::ostream& os, const majel::Dim<i>& d) {
std::ostream& os, const Dim<i>& d) {
os << d.head << ", " << d.tail;
return os;
}
Expand All @@ -420,7 +421,7 @@ typename std::enable_if<(i > 1), std::ostream&>::type operator<<(
// XXX I wish this could be an overload instead of a template
template <int i>
typename std::enable_if<(i == 1), std::ostream&>::type operator<<(
std::ostream& os, const majel::Dim<i>& d) {
std::ostream& os, const Dim<i>& d) {
os << d.head;
return os;
}
Expand Down Expand Up @@ -448,4 +449,5 @@ HOSTDEVICE Dim<D> linear_to_dimension(int linear_index, Dim<D> extents) {
return result;
}

} // namespace majel
} // namespace framework
} // namespace paddle
Loading