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.

Sign up for GitHub

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

Jump to bottom

implement conv3d op #4400

Merged
merged 5 commits into from
Dec 4, 2019
Merged

implement conv3d op #4400

merged 5 commits into from
Dec 4, 2019

Conversation

optima2005
Copy link
Contributor

This is a start attempt to implement #4009

The implemation in this PR is a very basic version of conv3d( NCDHW layout only). I am proposing this version to confirm that I am in the correct direction. And if so, I think I can go on or others can keep working it from this on.

I can run through the below testing on a x86 cpu and a nvidia gpu server, both linux OS.
TVM_FFI=ctypes python -m pytest -v tests/python/relay/test_op_level2.py -k test_conv3d_run
TVM_FFI=ctypes python -m pytest -v tests/python/contrib/test_cudnn.py -k test_conv3d

@masahi masahi self-assigned this Nov 22, 2019
masahi
masahi reviewed Nov 23, 2019
View reviewed changes
python/tvm/contrib/cudnn.py Outdated
filter_d,
filter_h,
filter_w):
"""Get weight shape for a 2D convolution
Copy link
Member

Choose a reason for hiding this comment

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

3D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix next commit

masahi
masahi reviewed Nov 23, 2019
View reviewed changes
python/tvm/contrib/cudnn.py Outdated
filter height
filter_w: int
filter width

Copy link
Member

Choose a reason for hiding this comment

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

filter_d

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix next commit

masahi
masahi reviewed Nov 23, 2019
View reviewed changes
python/tvm/contrib/cudnn.py Outdated
0: CUDNN_CONVOLUTION
1: CUDNN_CROSS_CORRELATION
tensor_format: int
0: CUDNN_TENSOR_NCHW
Copy link
Member

Choose a reason for hiding this comment

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

Is this value compatible with 3D convolution?

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. cudnn doesn't have specific tensor format for 3D and up dimentions convolution, the leading format is same "NCHW" and the additional dimentions would be append to the tail for 'channel first' format. Please see below citation from nvidia DL SDK

format

    Input.Type of the filter layout format. If this input is set to CUDNN_TENSOR_NCHW, which is one of the enumerant values allowed by cudnnTensorFormat_t descriptor, then the layout of the filter is as follows:

        For N=4, a 4D filter descriptor, the filter layout is in the form of KCRS:
            K represents the number of output feature maps
            C is the number of input feature maps
            R is the number of rows per filter
            S is the number of columns per filter
        For N=3, a 3D filter descriptor, the number S (number of columns per filter) is omitted.
        For N=5 and greater, the layout of the higher dimensions immediately follow RS.

    On the other hand, if this input is set to CUDNN_TENSOR_NHWC, then the layout of the filter is as follows:

        For N=4, a 4D filter descriptor, the filter layout is in the form of KRSC.
        For N=3, a 3D filter descriptor, the number S (number of columns per filter) is omitted and the layout of C immediately follows R.
        For N=5 and greater, the layout of the higher dimensions are inserted between S and C. For more information, see cudnnTensorFormat_t.

masahi
masahi reviewed Nov 23, 2019
View reviewed changes
src/relay/op/nn/convolution.cc
- **weight**: (channels, in_channels, kernel_size[0], kernel_size[1])
- **out**: This depends on the `layout` parameter. Output is 4D array of shape
(batch_size, channels, out_height, out_width) if `layout` is `NCHW`.

Copy link
Member

Choose a reason for hiding this comment

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

need to update document above for 3D

@masahi
Copy link
Member

masahi commented Nov 23, 2019

@optima2005 thank you very much for working on this. Can we use cuDNN's ND API like cudnnSetConvolutionNdDescriptor for 2D convolution? It would be great if we could unify 2D and 3D implementation.

@masahi
Copy link
Member

masahi commented Nov 23, 2019

@optima2005 can you add test cases to topi/tests/python too?

@optima2005
Copy link
Contributor Author

@masahi, glad to contribute! Many thanks for the review!

For "unify 2D and 3D" cudnn convolution, I am wondering whether it is better to keep separation. The APIs are 2 groups in cudnn lib(2D vs ND), I guess whether there would be some specific optimazation for 2D group. What do you think about that?

For other comments, I have revised in the latest commit, please check again, thanks a lot!

@masahi
Copy link
Member

masahi commented Nov 23, 2019 •
edited
Loading

I also wondered if there is a performance penalty in using ND api for 2D convolution. I looked at pytorch's implemention to see how they deal with ND convolution. It seems they are using ND API for all dimension, including 2D. See here and here.

@masahi
Copy link
Member

masahi commented Nov 23, 2019

So I think we can go with the ND API for both 2D and 3D convolution. If you are worried about performance you can always do benchmarks.

Can you first send a PR that refactors our cuDNN convolution to use the ND API? It will make reviewing this PR easier.

@optima2005 optima2005 mentioned this pull request Nov 25, 2019
Merged
@optima2005
Copy link
Contributor Author

@masahi, I raised #4418 that refactors the cuDNN convolution to use the ND API.

@optima2005
Copy link
Contributor Author

@masahi, I have rebased to the master. Please go on to review. Thanks!

@masahi masahi merged commit 7e32f37 into apache:master Dec 4, 2019
@masahi
Copy link
Member

masahi commented Dec 4, 2019

Thanks @optima2005 this is merged.

zxy844288792 pushed a commit to zxy844288792/tvm that referenced this pull request Dec 13, 2019
@optima2005
implement conv3d op (apache#4400)
6cde692 
* implement conv3d op

* add back missed conv2d_output_shape by mistake

* fix typo and docs, add topi test

* rebase to master and merge 2d/3d unification

* use cudnn.conv_forward
zxy844288792 pushed a commit to neo-ai/tvm that referenced this pull request Dec 13, 2019
@optima2005
implement conv3d op (apache#4400)
cc56dd8 
* implement conv3d op

* add back missed conv2d_output_shape by mistake

* fix typo and docs, add topi test

* rebase to master and merge 2d/3d unification

* use cudnn.conv_forward
@deepakbabel
Copy link

Hi @optima2005 , @masahi ,
I am new to tvm community. I was trying to implement maxpool3d operator based on reference implementation from tensorflow(r=1.14) into tvm codebase. For testing the same, I have written relay and topi test cases. I am also writing test cases in test_forward.py(for tensorflow) which compares tvm output with tensorflow output. I want to know is there any specific reason why you have not added any changes in test_forward.py(comparing tvm with tf) for unit testing?? Should i ignore this file altogether?

@zhiics zhiics mentioned this pull request Sep 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@masahi masahi masahi approved these changes

Assignees

@masahi masahi

Labels
status: need review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@optima2005 @masahi @deepakbabel @kevinthesun