Refactorize registry macro #3373
Conversation
… refactor_registry_macro
paddle/framework/op_registry.h
Outdated
| @@ -307,22 +307,38 @@ class OpRegistry { | |||
| } | |||
| }; | |||
|
|
|||
| class Registrar { | |||
| public: | |||
| void Touch() {} | |||
There was a problem hiding this comment.
We need a comment here for Touch.
There was a problem hiding this comment.
// In our design, various kinds of classes, e.g., operators and kernels, have their corresponding registry and registrar. The action of registration is in the constructor of a global registrar variable, which, however, are not used in the code that calls package framework, and would be removed from the generated binary file by the linker. To avoid such removal, we add Touch to all registrar classes and make USE_OP macros to call this method. So, as long as the callee code calls USE_OP, the global registrar variable won't be removed by the linker.
| template <typename OpType, typename ProtoMakerType> | ||
| class OpRegisterHelper { | ||
| class OpRegistrar : public Registrar { |
There was a problem hiding this comment.
It seems that using struct instead of class could save a line public:.
There was a problem hiding this comment.
In google code style:
"Use a struct only for passive objects that carry data; everything else is a class."
https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes
paddle/framework/op_registry.h
Outdated
| class OpKernelRegistrar : public Registrar { | ||
| public: | ||
| explicit OpKernelRegistrar(const char* op_type) { | ||
| ::paddle::framework::OperatorWithKernel::OpKernelKey key; |
There was a problem hiding this comment.
::paddle::framework::OperatorWithKernel
==>
OperatorWithKernel
paddle/framework/op_registry.h
Outdated
| } | ||
|
|
||
| /** | ||
| * Macro to Forbid user register Gradient Operator. | ||
| * Macro to Register OperatorKernel. |
| // `NO_GRAD` to disable micro USE_OP_GRADIENT(op_type). Otherwise the code can't | ||
| // be compiled. `NO_GRAD` should be removed after all gradient ops are | ||
| // compeleted. | ||
| #define NO_GRAD |
There was a problem hiding this comment.
这个有点儿危险。要是忘了删除 NO_GRAD 呢？
There was a problem hiding this comment.
其实在原来的代码中，完全就没有对gradient op被链接进来的保证，完全依靠将正反向op写在同一个文件里来把反向op链接进来。所以原来的代码里很多op没写反向也能编译链接通过，其实都是不正常的
| #endif | ||
|
|
||
| #define USE_NO_GRAD_OP(op_type) \ |
There was a problem hiding this comment.
I remember that #3319 have some proposals about macro names. How about using them?
There was a problem hiding this comment.
In my mind, we have two choices:
- USE_OP \ USE_OP_WITHOUT_KERNEL (our current design)
- USE_OP_WITH_KERNEL \ USE_OP (proposed in Refactorize op_registry.* #3319)
Actually, most operators have their kernels. So we will write left macro much more frequently than the right one. I think we'd better keep the frequently used macro short?
Fixes #3372
REGISTER_OP_KERNEL. And making it an independent classOpKernelRegistrar.ATTENTION: For many gradient operators have not been completed, the macro
REGISTER_OP_KERNEL()is disabled for the present.