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

Merge maps in OpRegistry and simplify register macros #3436

Merged
merged 12 commits into from
Aug 14, 2017

Conversation

JiayiFeng
Copy link
Collaborator

@JiayiFeng JiayiFeng commented Aug 11, 2017

In this PR, we fix #3435, and changed definitions of register macros:

// register an operator as well as its gradient version
REGISTER_OP(op_type, op_class, op_maker_class, grad_op_type, grad_op_class)

// register an operator without any gradient version
REGISTER_OP_WITHOUT_GRADIENT(op_type, op_class, op_maker_class)

And also there are some small alterations in USE_XXX macros:

// use an operator and all its available kernels
USE_OP(op_type)

// use an operator and its CPU kernels
USE_CPU_ONLY_OP(op_type)

// only use operator itself, no searching or linking job of its kernels
// e.g. RNN op
USE_OP_ITSELF(op_type)

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 TO REGISTER_OP AFTER FINISHING THEIR GRADIENTS.

@wangkuiyi
Copy link
Collaborator

It looks to me that we need only one of REGISTER_OP_WITHOUT_GRADIENT and REGISTER_GRADIENT_OP.

Say, if we decide to take REGISTER_OP_WITHOUT_GRADIENT, we can still have REGISTER_GRADIENT_OP, but let REGISTER_OP other than the final user to call it.

@@ -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) \
Copy link
Contributor

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.

Copy link
Collaborator

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.

STATIC_ASSERT_GLOBAL_NAMESPACE( \
__reg_gradient_op__##op_type##_##op_type##_grad, \
"NO_GRADIENT must be called in global namespace")
STATIC_ASSERT_GLOBAL_NAMESPACE( \
Copy link
Contributor

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.

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 mean macro NO_GRADIENT(op_type) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes. Is it redundant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed

@JiayiFeng JiayiFeng merged commit cdd1a8f into PaddlePaddle:develop Aug 14, 2017
@JiayiFeng JiayiFeng deleted the refactor_registry_macro branch August 14, 2017 22:22
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge maps in OpRegistry and simplify register macros
4 participants