Skip to content

Add Majel's Place concept to Paddle #2091

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

Merged
merged 1 commit into from
May 11, 2017

Conversation

wangkuiyi
Copy link
Collaborator

@wangkuiyi wangkuiyi commented May 10, 2017

Fixes #2090

Note: Please review and merge #2040 before this. In order to build the ported Majel code, I am using the development Docker image.

Dockerfile Outdated
curl sed grep graphviz libjpeg-dev zlib1g-dev \
python-numpy python-matplotlib gcc g++ \
automake locales clang-format-3.8 swig doxygen cmake \
liblapack-dev liblapacke-dev libboost-dev \
Copy link
Contributor

@gangliao gangliao May 11, 2017

Choose a reason for hiding this comment

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

@wangkuiyi It's better to replace boost using some light-weight implementation https://github.com/mapbox/variant since it's too heavy.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/PaddlePaddle/Paddle/blob/develop/cmake/external/glog.cmake#L29 It's a good way to integrate external project via CMake.

Copy link
Collaborator Author

@wangkuiyi wangkuiyi May 11, 2017

Choose a reason for hiding this comment

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

About boost

I listed all boost packages that Majel depends on:

$ for i in $(du -a | grep '\.h$' | cut -f 2); do \
    grep 'boost::' $i; \
  done | \
 grep -o 'boost::[a-zA-Z_]*' | sort | uniq
boost::apply_visitor
boost::bad_get
boost::get
boost::python
boost::shared_ptr
boost::static_visitor
boost::variant

I am afraid that we cannot replace all of them using some lighter weighted versions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I understand and agree that we should use CMake and its external projects. I didn't do it in this PR because I want to figure out what is the command line we need to build Majel.

@@ -0,0 +1,10 @@
CCFLAGS = -std=c++11 -I/work/paddle
Copy link
Contributor

Choose a reason for hiding this comment

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

After this PR is merged, I will add a CMakeLists.txt.

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

@gangliao gangliao merged commit 49d70c4 into PaddlePaddle:develop May 11, 2017
// needed for variant equality comparison
inline bool operator==(const GpuPlace& o) const { return device == o.device; }

inline bool operator!=(const GpuPlace& o) const { return !(*this == o); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the above "==" operation check equal as device == o.device, but here checks if it's the same instance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right! Is Greg nearby and can you ask him about the problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, I thought *this is this. In this case, it's taking the neg of result from == operator.

Copy link
Contributor

@gangliao gangliao May 12, 2017

Choose a reason for hiding this comment

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

@wangkuiyi @helinwang I think it's correct. Because *this == o will trigger inline bool operator==(const GpuPlace& o) So it's still compare device id.

@wangkuiyi wangkuiyi deleted the majel-place branch May 12, 2017 14:42
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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants