Skip to content

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
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

3 participants