Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix use of convdims. #7

Merged
merged 2 commits into from
Apr 29, 2021
Merged

Fix use of convdims. #7

merged 2 commits into from
Apr 29, 2021

Conversation

maleadt
Copy link
Contributor

@maleadt maleadt commented Apr 29, 2021

JuliaGPU/CUDA.jl#873 changed CUDNN.convdims; replicating that change here. This has entered the CUDA 3.1 release, so users will run into this.

That said, NNlibCUDA shouldn't be calling these methods, and instead use the high-level CUDNN API. Currently it has just copied the old implementation that calls into low-level wrappers, while using unexported helper functions like CUDNN.convdims to do so.

Copy link
Member

@DhairyaLGandhi DhairyaLGandhi left a comment

Choose a reason for hiding this comment

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

Thank you so much! Should we also copy some tests to catch the instances that the current changes were meant to catch? We can do so in a follow up, but thinking about the correct way forward here.

@maleadt
Copy link
Contributor Author

maleadt commented Apr 29, 2021

I guess NNlibCUDA should call the public cudnnConvolutionForward method, which creates the descriptor based on the other keyword arguments, instead of creating the descriptor manually here. That code is then covered by the CUDA.jl tests.

Co-authored-by: Dhairya Gandhi <dhairya@juliacomputing.com>
@DhairyaLGandhi
Copy link
Member

Thanks @maleadt and @DrChainsaw

@DhairyaLGandhi DhairyaLGandhi merged commit 25631eb into master Apr 29, 2021
@DhairyaLGandhi DhairyaLGandhi deleted the tb/convdims branch April 29, 2021 10:44
@DhairyaLGandhi DhairyaLGandhi restored the tb/convdims branch April 29, 2021 10:49
@DhairyaLGandhi
Copy link
Member

Makes sense to split the public bits from the descriptor handling.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants