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

Op refine for making build system more automatic #3881

Merged
merged 9 commits into from
Sep 6, 2017
Merged

Op refine for making build system more automatic #3881

merged 9 commits into from
Sep 6, 2017

Conversation

luotao1
Copy link
Contributor

@luotao1 luotao1 commented Sep 5, 2017

fix #3807 (comment)
fix #3872 : As there will be more only-CPU ops, it is not necessary to message these lines.
related #3852 .

@luotao1 luotao1 requested a review from typhoonzero September 5, 2017 10:22
@luotao1
Copy link
Contributor Author

luotao1 commented Sep 5, 2017

@typhoonzero As the commit info of #3852 is mass, I create a new PR, and all your commens are Done.

@luotao1 luotao1 changed the title Op refine Op refine for making build system more automatic Sep 5, 2017
@typhoonzero
Copy link
Contributor

Seem ci failed at:

CMakeFiles/grad_op_builder_test.dir/grad_op_builder_test.cc.o: In function `__static_initialization_and_destruction_0(int, int) [clone .constprop.318]':
[20:05:40]	grad_op_builder_test.cc:(.text.startup+0x86): undefined reference to `TouchOpKernelRegistrar_add_GPU()'
[20:05:40]	collect2: error: ld returned 1 exit status

@luotao1
Copy link
Contributor Author

luotao1 commented Sep 5, 2017

I see it, I am trying to fix it.

@luotao1
Copy link
Contributor Author

luotao1 commented Sep 5, 2017

fix the ci error, and remove unnecessary .h for add_op.cu

// identity is a alias of scale op. This is also a example for creating a alias
// operator.
template <typename AttrType>
class IdentityOpMaker : public framework::OpProtoAndCheckerMaker {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why remove the identity_op?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@reyoung 只是把 identity_op弄成了一个单独的文件,没有放在scale_op里面,并没有删掉它。这样CMakeLists.txt能自动根据operators/目录下的文件名,来获得有哪些op需要编译,之后也方便自动生成pybind.cc中的USE_OP(XXX),而不用一个个手写添加。

list(LENGTH cu_srcs cu_srcs_len)
list(LENGTH op_library_DEPS dep_len)
if (${cu_srcs_len} EQUAL 0 AND ${dep_len} EQUAL 0)
message(WARNING "The op library ${TARGET} not support GPU!")
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove message is fine, because sometimes we can implement the CPU version only and then implement GPU version.

Copy link
Contributor

@typhoonzero typhoonzero left a comment

Choose a reason for hiding this comment

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

LGTM!

@luotao1 luotao1 merged commit 50870a6 into PaddlePaddle:develop Sep 6, 2017
@luotao1 luotao1 deleted the op_refine branch September 6, 2017 07:03
heavengate added 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.

Cmake warning on develop branch Simplify operator development by making build system more automatic
3 participants