-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Merge maps in OpRegistry and simplify register macros #3436
Merge maps in OpRegistry and simplify register macros #3436
Conversation
… refactor_registry_macro_dev
It looks to me that we need only one of Say, if we decide to take |
@@ -358,30 +357,20 @@ class OpKernelRegistrar : public Registrar { | |||
/** | |||
* Macro to register Operator. | |||
*/ | |||
#define REGISTER_OP(op_type, op_class, op_maker_class) \ | |||
#define REGISTER_OP(op_type, op_class, op_maker_class, grad_op_type, \ | |||
grad_op_class) \ |
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.
when we change the registration style, we need to discuss a previous problem. The user's code needs to write USE_OP(OP)
, USE_OP(OP_GRAD)
twice. It is annoying.
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.
Just USE_OP(OP)
is OK. because we always register forward operator and backward operator in same .cc/cu
file.
USE_OP(OP)
will force C++ linker to include all symbols in that .cc/cu
file including forwarding operator and backwarding operator.
paddle/framework/op_registry.h
Outdated
STATIC_ASSERT_GLOBAL_NAMESPACE( \ | ||
__reg_gradient_op__##op_type##_##op_type##_grad, \ | ||
"NO_GRADIENT must be called in global namespace") | ||
STATIC_ASSERT_GLOBAL_NAMESPACE( \ |
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.
we already have a unified registrar REGISTER_OP_WITHOUT_GRADIENT
. I think we does not need this one any 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.
You mean macro NO_GRADIENT(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.
yes. Is it redundant?
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.
Removed
… refactor_registry_macro
… refactor_registry_macro
In this PR, we fix #3435, and changed definitions of register macros:
And also there are some small alterations in
USE_XXX
macros:Most operators have corresponding gradient operators, but in our code, some of them have not been completed. To pass compiling, I have to register these forward-only operators by
REGISTER_OP_WITHOUT_GRADIENT
for the moment. PLEASE AMEND THEM TOREGISTER_OP
AFTER FINISHING THEIR GRADIENTS.