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

[Don't merge] Refactor grouped convolution #1255

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

bartekxk
Copy link
Contributor

  • Remove 1d and 2d grouped convolution instances
  • Move MakeDescriptor.... functions to transform headers

Copy link
Contributor

@iq136boy iq136boy left a comment

Choose a reason for hiding this comment

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

May I ask when CK will deprecate the support of 2d convolution instances? This has a significant impact on MIOpen side because there are several solvers in MIOpen based on 2d ck instances and APIs. They are in the rocm6.2 mainline staging and (at lease some of them) are now being in tuning process or has done the tuning.

@bartekxk
Copy link
Contributor Author

May I ask when CK will deprecate the support of 2d convolution instances? This has a significant impact on MIOpen side because there are several solvers in MIOpen based on 2d ck instances and APIs. They are in the rocm6.2 mainline staging and (at lease some of them) are now being in tuning process or has done the tuning.

Hi @iq136boy , the reason is to reduce number of instances. 3D instances can handle 1d and 2d cases (for example, for 2d user can set Z=1, Di=1, Do=1, d_padding=0, d_stride=1, d_dilation=1). Of course, if MIOpen still uses 2d instances this change could be suspended until MIOpen will update instances to 3d.

@bartekxk bartekxk changed the title Refactor grouped convolution [Don't merge] Refactor grouped convolution Apr 22, 2024
@iq136boy
Copy link
Contributor

May I ask when CK will deprecate the support of 2d convolution instances? This has a significant impact on MIOpen side because there are several solvers in MIOpen based on 2d ck instances and APIs. They are in the rocm6.2 mainline staging and (at lease some of them) are now being in tuning process or has done the tuning.

Hi @iq136boy , the reason is to reduce number of instances. 3D instances can handle 1d and 2d cases (for example, for 2d user can set Z=1, Di=1, Do=1, d_padding=0, d_stride=1, d_dilation=1). Of course, if MIOpen still uses 2d instances this change could be suspended until MIOpen will update instances to 3d.

Yes. We do need this changes to be suspended until we updated the MIOpen solvers to using only 3d ck instances.

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.

2 participants