Skip to content

More about cudnn refactor#50827

Closed
zasdfgbnm wants to merge 7 commits intomasterfrom
conv-fix
Closed

More about cudnn refactor#50827
zasdfgbnm wants to merge 7 commits intomasterfrom
conv-fix

Conversation

@zasdfgbnm
Copy link
Copy Markdown
Collaborator

  • Resolves @ngimel's review comments in Refactor cudnn convolution #49109
  • Move ConvolutionArgs from ConvShared.h to Conv_v7.cpp, because cuDNN v8 uses different descriptors therefore will not share the same ConvolutionArgs.
  • Refactor the ConvolutionParams (the hash key for benchmark):
    • Remove input_stride
    • Add input_dim
    • Add memory_format
  • Make repro_from_args to take ConvolutionParams instead of ConvolutionArgs as arguments so that it can be shared for v7 and v8
  • Rename some layout to memory_format. layout should be sparse/strided and memory_format should be contiguous/channels_last. They are different things.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 20, 2021

Codecov Report

Merging #50827 (c299089) into master (22902b9) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #50827      +/-   ##
==========================================
- Coverage   81.09%   81.08%   -0.02%     
==========================================
  Files        1915     1915              
  Lines      208980   208980              
==========================================
- Hits       169464   169441      -23     
- Misses      39516    39539      +23     

@ngimel ngimel added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jan 21, 2021
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@ezyang merged this pull request in 75cba9d.

@zasdfgbnm zasdfgbnm deleted the conv-fix branch January 25, 2021 21:18
@zasdfgbnm zasdfgbnm mentioned this pull request May 17, 2021
9 tasks
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
Summary:
- Resolves ngimel's review comments in pytorch#49109
- Move `ConvolutionArgs` from `ConvShared.h` to `Conv_v7.cpp`, because cuDNN v8 uses different descriptors therefore will not share the same `ConvolutionArgs`.
- Refactor the `ConvolutionParams` (the hash key for benchmark):
  - Remove `input_stride`
  - Add `input_dim`
  - Add `memory_format`
- Make `repro_from_args` to take `ConvolutionParams` instead of `ConvolutionArgs` as arguments so that it can be shared for v7 and v8
- Rename some `layout` to `memory_format`. `layout` should be sparse/strided and `memory_format` should be contiguous/channels_last. They are different things.

Pull Request resolved: pytorch#50827

Reviewed By: bdhirsh

Differential Revision: D26048274

Pulled By: ezyang

fbshipit-source-id: f71aa02d90ffa581c17ab05b171759904b311517
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla signed Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants