Skip to content

Comments

"refine kernel registrar"#6998

Merged
dzhwinter merged 11 commits intoPaddlePaddle:developfrom
dzhwinter:feature/multikey_register
Dec 27, 2017
Merged

"refine kernel registrar"#6998
dzhwinter merged 11 commits intoPaddlePaddle:developfrom
dzhwinter:feature/multikey_register

Conversation

@dzhwinter
Copy link
Contributor

No description provided.

reyoung
reyoung previously approved these changes Dec 26, 2017
Copy link
Collaborator

@reyoung reyoung left a comment

Choose a reason for hiding this comment

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

Basically LGTM. I have to read more recent PRs to understand whole logic, but do not let this block the merge.

QiJune
QiJune previously approved these changes Dec 27, 2017
Copy link
Member

@QiJune QiJune left a comment

Choose a reason for hiding this comment

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

LGTM

kMKLDNN = 1,
kCUDNN = 2,
kCPU = 3,
kGPU = 4
Copy link
Member

Choose a reason for hiding this comment

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

kGPU to kCUDA

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will fix in next PR.

kPlain = 0,
kMKLDNN = 1,
kCUDNN = 2,
kCPU = 3,
Copy link
Member

Choose a reason for hiding this comment

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

Here kPlain means the default library both in CPU and CUDA. Since we already have CPUPlace and CUDAPlace, we have no need to add kCPU library and kCUDA library. <CPUPlace, kPlain> and <CUDAPlace, kPlain> are the most common Operator kernels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make sense. done.

"REGISTER_OP_KERNEL must be called in global namespace"); \
static ::paddle::framework::OpKernelRegistrar<place_class, __VA_ARGS__> \
__op_kernel_registrar_##op_type##_##DEVICE_TYPE##__(#op_type); \
__op_kernel_registrar_##op_type##_##DEVICE_TYPE##__(#op_type, \
Copy link
Member

Choose a reason for hiding this comment

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

DEVICE_TYPE here is actually LIBRARY_TYPE. We can fix it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is.

Copy link
Member

@QiJune QiJune left a comment

Choose a reason for hiding this comment

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

LGTM

@dzhwinter dzhwinter merged commit 35c1683 into PaddlePaddle:develop Dec 27, 2017
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.

3 participants