Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates and unifies pipeline-related enums across the builder framework, replacing multiple specialized enum types (BlockGemmPipelineVersion, GridwiseGemmPipelineVersion, BlockGemmPipelineScheduler, LoopScheduler) with two unified enums (PipelineVersion and PipelineScheduler).
Key changes:
- Unified enum types for pipeline configuration that simplify the type system
- Added new backward convolution specialization and GEMM padding enums to support comprehensive reflection
- Introduced a new
ConvTraitsreflection infrastructure for extracting compile-time kernel properties - Added comprehensive test coverage for the new
ConvTraitsfunctionality - Included necessary header includes (
<string>) to support the new reflection code
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
experimental/builder/include/ck_tile/builder/types.hpp |
Replaced multiple pipeline-related enums with unified PipelineVersion and PipelineScheduler enums; added ConvBwdDataSpecialization, ConvBwdWeightSpecialization, and GemmPadding enums |
experimental/builder/include/ck_tile/builder/conv_factory.hpp |
Updated all references from old enum types to unified PipelineVersion and PipelineScheduler |
experimental/builder/include/ck_tile/builder/conv_algorithm_concepts.hpp |
Updated concept requirements to use unified enum types |
experimental/builder/include/ck_tile/builder/reflect/conv_traits.hpp |
New file implementing comprehensive compile-time reflection for extracting kernel properties from device instances |
experimental/builder/include/ck_tile/builder/reflect/instance_traits.hpp |
Removed redundant includes, keeping only essential headers |
experimental/builder/include/ck_tile/builder/reflect/instance_traits_util.hpp |
Added case-insensitive string comparison utility and necessary includes |
experimental/builder/include/ck_tile/builder/reflect/instance_traits_device_grouped_conv_fwd_multiple_abd_xdl_cshuffle_v3.hpp |
Added include for instance_traits_util.hpp |
experimental/builder/include/ck_tile/builder/reflect/instance_traits_device_grouped_conv_fwd_multiple_d_wmma_cshuffle.hpp |
Added include for instance_traits_util.hpp |
experimental/builder/test/impl/conv_algorithm_types.hpp |
Updated struct definitions to use unified enum types |
experimental/builder/test/utils/ckb_conv_test_common.hpp |
Updated test utilities to use unified enum types |
experimental/builder/test/conv/test_ckb_conv_fwd_*.cpp |
Updated test instantiations to use PipelineVersion instead of BlockGemmPipelineVersion |
experimental/builder/test/conv/test_conv_traits.cpp |
New comprehensive test file for ConvTraits reflection functionality |
experimental/builder/test/CMakeLists.txt |
Added new test target for test_conv_traits.cpp and duplicate test file entry |
include/ck/tensor_operation/gpu/device/convolution_backward_weight_specialization.hpp |
Added missing <string> include |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
experimental/builder/include/ck_tile/builder/reflect/instance_traits_util.hpp
Outdated
Show resolved
Hide resolved
experimental/builder/include/ck_tile/builder/reflect/instance_traits_util.hpp
Outdated
Show resolved
Hide resolved
| V3, // Only used in stream-K implementation | ||
| V4, | ||
| V5, | ||
| WEIGHT_ONLY |
There was a problem hiding this comment.
If I remember correctly it is some Navi specific pipeline for inference.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| @@ -0,0 +1,719 @@ | |||
| // SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
If you make changes to this PR, it would be helpful to update this to match the new style (no year, copyright on first line).
|
Where do we want to make the jump from specialized (templated) structs to just a standard struct? My original thought was this workflow: InstanceTraits -> MakeConvTraits() -> ConvTraits The logic is:
In this PR, the MakeConvTraits is built into the ConvTraits specializaiton, and I don't know where we end the metaprogramming. I don't think there is a runtime case for making all the reflection code build time metaprogramming. Shouldn't we get back to simple C++ design sooner rather than later? |
@shumway Isn't a ConvDescription designed for that? That would be in next PR. The |
shumway
left a comment
There was a problem hiding this comment.
This looks good, let's get this in before changing more.
I really think we want to the Convolution traits structs to be non-templated, just to make the descriptor code easier to write, but if we make that change, that should come after this PR.
Proposed changes
Added: