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

Add pool2d cudnn op #4636

Merged
merged 6 commits into from
Oct 27, 2017
Merged

Conversation

chengduoZH
Copy link
Contributor

@chengduoZH chengduoZH commented Oct 9, 2017

fix #4637
Add pool cudnn op.

  • Finish the cudnn pool2d maxpool and avgpool, only support NCHW(data layout) .
  • Finish the cudnn pool2d unit test.
  • Fix pool_op attribution's name.
  • TODO in next PR:
    • Add cudnn pool3d maxpool and avgpool.
    • Add cudnn pool3d unit test.
    • Support NHWC(data layout) .

@chengduoZH chengduoZH force-pushed the Add_pool_cudnn_op branch 2 times, most recently from 75a2f0f to 104ca74 Compare October 9, 2017 11:42
@chengduoZH chengduoZH changed the title Add pool cudnn op [WIP]Add pool cudnn op Oct 9, 2017
@chengduoZH chengduoZH force-pushed the Add_pool_cudnn_op branch 2 times, most recently from 846e479 to 8c51a0d Compare October 9, 2017 11:54
@chengduoZH chengduoZH force-pushed the Add_pool_cudnn_op branch 7 times, most recently from 92b75fd to 91caf62 Compare October 10, 2017 03:22
@chengduoZH chengduoZH changed the title [WIP]Add pool cudnn op Add pool cudnn op Oct 10, 2017
@chengduoZH chengduoZH force-pushed the Add_pool_cudnn_op branch 4 times, most recently from 80f8bf7 to 4b82b2f Compare October 11, 2017 11:28
@chengduoZH chengduoZH force-pushed the Add_pool_cudnn_op branch 5 times, most recently from e5d7580 to 926b90f Compare October 25, 2017 15:58
@chengduoZH chengduoZH force-pushed the Add_pool_cudnn_op branch 3 times, most recently from 0b12868 to 74abe6b Compare October 26, 2017 14:21
@chengduoZH chengduoZH force-pushed the Add_pool_cudnn_op branch 2 times, most recently from 28b534c to 8149e6d Compare October 27, 2017 01:55
@chengduoZH chengduoZH force-pushed the Add_pool_cudnn_op branch 2 times, most recently from 4e7c1de to 2d7d43f Compare October 27, 2017 02:18
@chengduoZH chengduoZH force-pushed the Add_pool_cudnn_op branch 4 times, most recently from 1c7f783 to 7f34029 Compare October 27, 2017 02:52
@chengduoZH chengduoZH changed the title Add pool cudnn op Add pool2d cudnn op Oct 27, 2017
@@ -80,34 +80,31 @@ Pool2dOpMaker::Pool2dOpMaker(framework::OpProto *proto,
"the number of channels, H and W is the height and "
"width of feature.");

AddAttr<std::string>("pooling_type",
"Pooling_type of pooling operator."
AddAttr<std::string>("poolingType",
Copy link
Contributor

Choose a reason for hiding this comment

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

Google C++ style did not specify the constant string naming rules, but since this attribute is going to be used when user define operators using python, following python style seems resonable?

AddAttr<std::string>("pooling_type",
"Pooling_type of pooling operator."
AddAttr<std::string>("poolingType",
"(string), poolingType of pooling operator."
Copy link
Contributor

Choose a reason for hiding this comment

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

(string), pooling type, can be "max" for max-pooling and "avg" for average-pooling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

"The pooling window size(height, width) of pooling operator."
"If global_pooling = true, ksize is ignored and need not be "
"(vector ), the pooling window size(height, width) of pooling operator."
"If globalPooling = true, ksize is ignored and need not be "
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove "and need not be specified".

Ignored means whether a user is providing this value, it's not used.

"need not be specified" means the user should never provide this value, or else it will cause error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

AddAttr<std::vector<int>>(
"ksize",
"The pooling window size(depth, height, width) of pooling operator."
"If global_pooling = true, ksize is ignored and need not be "
"(vector ), the pooling window size(depth, height, width) of pooling "
Copy link
Contributor

Choose a reason for hiding this comment

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

Pooling 3d and 2d seem to have very similar description documents, They can be merged into one OpMaker using the same description, like:

(vector), the pooling window size of pooling operator, for 3d pooling the layout should be (depth, height, width), for 2d it should be (height, width). ...

using PoolingMode = platform::PoolingMode;

// NOTE: copy from conv_cudnn
std::vector<int> Dims2VectorPool(const framework::DDim &dims) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can put this function to ddim.cc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


namespace paddle {
namespace operators {} // namespace operators
} // namespace paddle
Copy link
Contributor

Choose a reason for hiding this comment

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

Can remove this file and move #include to pool_cudnn_op.cu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, this header file is necessary to register as two op (pool2d_cudnn, pool2d_cudnn).
We can talk abort this in next PR.

@typhoonzero
Copy link
Contributor

The change of attr name causes CI error:

235/239 Test #236: test_recognize_digits_conv ..................***Failed 0.70 sec
[11:33:26]	Traceback (most recent call last):
[11:33:26]	File "test_recognize_digits_conv.py", line 35, in <module>
[11:33:26]	init_program=init_program)
[11:33:26]	File "/paddle/build/python/build/lib-python/paddle/v2/framework/nets.py", line 26, in simple_img_conv_pool
[11:33:26]	init_program=init_program)
[11:33:26]	File "/paddle/build/python/build/lib-python/paddle/v2/framework/layers.py", line 273, in pool2d
[11:33:26]	"paddings": pool_padding
[11:33:26]	File "/paddle/build/python/build/lib-python/paddle/v2/framework/layer_helper.py", line 44, in append_op
[11:33:26]	return self.program.current_block().append_op(*args, **kwargs)
[11:33:26]	File "/paddle/build/python/build/lib-python/paddle/v2/framework/framework.py", line 359, in append_op
[11:33:26]	op = Operator(self, op_desc, *args, **kwargs)
[11:33:26]	File "/paddle/build/python/build/lib-python/paddle/v2/framework/framework.py", line 263, in __init__
[11:33:26]	self.desc.check_attrs()
[11:33:26]	paddle.v2.framework.core.EnforceNotMet: Attribute 'poolingType' is required! at [/paddle/paddle/framework/attribute.h:215]
[11:33:26]	PaddlePaddle Call Stacks:
[11:33:26]	0 0x7f0aaaaf321cp paddle::platform::EnforceNotMet::EnforceNotMet(std::__exception_ptr::exception_ptr, char const*, int) + 572

Copy link
Contributor

@typhoonzero typhoonzero left a comment

Choose a reason for hiding this comment

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

LGTM++

@chengduoZH chengduoZH merged commit 5a95792 into PaddlePaddle:develop Oct 27, 2017
@chengduoZH chengduoZH added this to Done in Port Operators Oct 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Add pool cudnn op
2 participants