-
Notifications
You must be signed in to change notification settings - Fork 249
[CK TILE] Refactor Conv configs and Conv Elementwise #3151
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the grouped convolution code to improve API design by removing the CDElementwise template parameter from GroupedConvTraits and moving it to the kernel arguments level. Additionally, it renames GemmConfig to ConvConfig throughout the codebase for better clarity, and removes unused configuration fields (PermuteA, PermuteB, UseStructuredSparsity, Preshuffle, TiledMMAPermuteN).
- Moved
CDElementwisefromGroupedConvTraitstoGroupedConvFwdKernelArgstemplate parameters - Renamed
GemmConfigtoConvConfigacross all example files - Removed unused configuration parameters and simplified
TileGemmShapeandTileGemmUniversalTraitsinstantiations
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| include/ck_tile/ops/grouped_convolution/utils/grouped_convolution_utils.hpp | Removed CDElementwise_ template parameter from GroupedConvTraits |
| include/ck_tile/ops/grouped_convolution/kernel/grouped_convolution_forward_kernel.hpp | Added CDElementwise_ to GroupedConvFwdKernelArgs template parameters |
| example/ck_tile/20_grouped_convolution/run_grouped_convolution_fwd_example.inc | Renamed GemmConfig to ConvConfig |
| example/ck_tile/20_grouped_convolution/run_grouped_convolution_bwd_data_example.inc | Renamed GemmConfig to ConvConfig |
| example/ck_tile/20_grouped_convolution/grouped_convolution_forward_large_tensor_invoker.hpp | Renamed GemmConfig to ConvConfig, removed unused parameters, added duplicate comment |
| example/ck_tile/20_grouped_convolution/grouped_convolution_forward_large_tensor.cpp | Renamed GemmConfig to ConvConfig |
| example/ck_tile/20_grouped_convolution/grouped_convolution_forward_invoker.hpp | Renamed GemmConfig to ConvConfig, removed unused parameters, simplified trait instantiations |
| example/ck_tile/20_grouped_convolution/grouped_convolution_forward_bias_clamp.cpp | Renamed GemmConfig to ConvConfig |
| example/ck_tile/20_grouped_convolution/grouped_convolution_forward.cpp | Renamed GemmConfig to ConvConfig |
| example/ck_tile/20_grouped_convolution/grouped_convolution_backward_weight_two_stage_invoker.hpp | Removed unused parameters from TileGemmShape instantiation |
| example/ck_tile/20_grouped_convolution/grouped_convolution_backward_weight_invoker.hpp | Removed unused parameters from TileGemmShape instantiation |
| example/ck_tile/20_grouped_convolution/grouped_convolution_backward_data_invoker.hpp | Renamed GemmConfig to ConvConfig, removed unused parameters |
| example/ck_tile/20_grouped_convolution/grouped_convolution_backward_data.cpp | Renamed GemmConfig to ConvConfig |
| example/ck_tile/20_grouped_convolution/conv_configs.hpp | Removed unused configuration fields |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auto kargs = Kernel::MakeKernelArgs(args); | ||
|
|
||
| // Populate split-image metadata ONLY if using split-image kernel | ||
| // Populate split-image metadata ONLY if using split-image kernel |
Copilot
AI
Nov 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate comment on consecutive lines. Remove one of the duplicate comments.
| // Populate split-image metadata ONLY if using split-image kernel |
Proposed changes
Refactor Conv configs and Conv Elementwise. Change GemmConfigs to ConvConfigs and remove Elementwise from Conv Traits. Remove not needed members
Checklist
Please put an
xinto the boxes that apply. You can also fill these out after creating the PR. If you're not sure, please don't hesitate to ask.clang-formaton all changed filesDiscussion
If this is a relatively large or complex change, feel free to start a discussion by explaining why you chose the solution you did and what alternatives you considered