-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 a sample op, add_op
#2827
Add a sample op, add_op
#2827
Conversation
84ac1af
to
ab48ef8
Compare
paddle/op/add_op.h
Outdated
namespace paddle { | ||
namespace op { | ||
|
||
template <typename Place> |
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.
这里的模板参数应该有两个吧,一个是设备类型,一个是数据类型
template <typename DeviceType, typename DataType>
class AddKernel
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.
目前既然没有不同数据类型的实现,就先不考虑不同数据类型的问题。
__op_register_##__op_type##__(#__op_type); \ | ||
int __op_register_##__op_type##_handle__() { return 0; } | ||
|
||
#define REGISTER_OP_KERNEL(type, GPU_OR_CPU, PlaceType, KernelType) \ |
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.
这里目前先不考虑对不同数据类型kernel的支持吗?
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.
目前既然没有不同数据类型的实现,就先不考虑不同数据类型的问题。
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.
会有double
、float
、half
等数据类型的支持吧,希望从接口下考虑下。
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.
如果需要增加数据类型的话,只是注册的时候加一个参数就好了。除了注册之外的接口没有区别。
ab48ef8
to
7bf260a
Compare
* Refine register methods, make Op can get rid of whole-archieve * `USE_OP` before a op is used. * Add unittest for add_op.
7bf260a
to
a0aaafe
Compare
paddle/CMakeLists.txt
Outdated
@@ -14,6 +14,7 @@ if(Boost_FOUND) | |||
add_subdirectory(memory) | |||
add_subdirectory(platform) | |||
add_subdirectory(framework) | |||
add_subdirectory(op) # because `operator` is a reserved word for CPP, so short to `op` |
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.
a personal suggestion, use ops
instead of op
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.
Maybe operators
is better.
@@ -101,8 +102,11 @@ class OpRegistry { | |||
OpProto& op_proto = protos()[op_type]; | |||
OpAttrChecker& op_checker = op_checkers()[op_type]; | |||
ProtoMakerType(&op_proto, &op_checker); | |||
PADDLE_ENFORCE(op_proto.IsInitialized(), | |||
"Fail to initialize %s's OpProto !", op_type); | |||
*op_proto.mutable_type() = op_type; |
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.
I am curious why this can work before without set op_type
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.
LGTM!
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.
LGTM. but device_type
is better than CPU_OR_GPU
* Convert `op` --> `operators` * Remove AddType in OpProtoMaker, because type is part of registry. * Rename CPU_OR_GPU --> DEVICE_TYPE in registry macro.
"REGISTER_OP must be in global namespace"); \ | ||
static ::paddle::framework::OpRegisterHelper<__op_class, __op_maker_class> \ | ||
__op_register_##__op_type##__(#__op_type); \ | ||
int __op_register_##__op_type##_handle__() { return 0; } |
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.
这里加上这个函数是什么作用啊?
int __op_register_##__op_type##_handle__() { return 0; }
USE_OP
before a op is used.