Skip to content

Conversation

@ecamartins
Copy link
Collaborator

@ecamartins ecamartins commented Oct 29, 2025

Currently, in various CK Tile examples, there is a use of pipeline macros of the following form

#define CK_TILE_PIPELINE_COMPUTE_V3 1
#define CK_TILE_PIPELINE_MEMORY 2
#define CK_TILE_PIPELINE_COMPUTE_V4 3
#define CK_TILE_PIPELINE_COMPUTE_V5 4
#define CK_TILE_PIPELINE_COMPUTE_V6 5
#define CK_TILE_PIPELINE_PRESHUFFLE_V1 6
#define CK_TILE_PIPELINE_PRESHUFFLE_V2 7

That said, these macros are duplicated across examples.

Thus, this PR extracts this common functionality into include/ck_tile/ops/gemm/pipeline/gemm_pipelines.hpp and converted the macro to a GemmPipeline enum instead.

I have compiled and run all modified examples locally. All compile and run successfully except tile_example_grouped_gemm_preshuffle and tile_example_grouped_conv_fwd_bias_clamp. These examples fails to compile on develop without my changes, so I will log tickets for these cases.

Proposed changes

Please describe the motivation behind the pull request, whether it enables a new feature or fixes a bug. If there are associated pull requests or issues, please link them to the pull request.

Checklist

Please put an x into 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.

  • I have added tests relevant to the introduced functionality, and the unit tests are passing locally
  • I have added the test to REGRESSION_TESTS list defined at the top of CMakeLists.txt in tests/CMakeLists.txt, IF the test takes more than 30 seconds to run.
  • I have added inline documentation which enables the maintainers with understanding the motivation
  • I have removed the stale documentation which is no longer relevant after this pull request
  • (If this change is user-facing) I have added release notes which provide the end users with a brief summary of the improvement from this pull request
  • I have run clang-format on all changed files
  • Any dependent changes have been merged

Copy link
Contributor

Copilot AI left a 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 GEMM pipeline configuration system by replacing preprocessor macro definitions with a strongly-typed enum class. The change removes hardcoded integer macros (CK_TILE_PIPELINE_COMPUTE_V3, etc.) and introduces a new ck_tile::GemmPipeline enum with named values (e.g., ck_tile::GemmPipeline::COMPUTE_V3).

Key changes:

  • Introduces new ck_tile::GemmPipeline enum class with pipeline variant names
  • Replaces all macro-based pipeline identifiers with enum values throughout the codebase
  • Updates template parameters from ck_tile::index_t to ck_tile::GemmPipeline for type safety

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/gemm/pipeline/gemm_pipelines.hpp Defines new GemmPipeline enum class with all pipeline variants
include/ck_tile/ops/gemm.hpp Adds include for the new gemm_pipelines.hpp header
test/ck_tile/gemm/test_gemm_pipeline_smoke_util.hpp Removes macro definitions, updates Pipeline field types and PipelineTypeTraits template parameter
example/ck_tile/22_gemm_multi_abd/gemm_multi_abd_fp16.hpp Removes macro definitions, updates Pipeline field types and PipelineTypeTraits template parameter
example/ck_tile/20_grouped_convolution/conv_configs.hpp Removes macro definitions, updates Pipeline field types and PipelineTypeTraits template parameter
example/ck_tile/19_gemm_multi_d/gemm_multi_d_fp16.hpp Removes macro definitions, updates Pipeline field types and PipelineTypeTraits template parameter
example/ck_tile/17_grouped_gemm/quant_grouped_gemm.hpp Removes macro definitions, updates Pipeline field types, adds missing Pipeline field to GemmConfigComputeV3_2
example/ck_tile/17_grouped_gemm/grouped_gemm_multi_d.hpp Removes macro definitions, updates Pipeline field types and PipelineTypeTraits template parameter
example/ck_tile/17_grouped_gemm/grouped_gemm.hpp Removes macro definitions, updates Pipeline field types and PipelineTypeTraits template parameter
example/ck_tile/16_batched_gemm/batched_gemm.hpp Removes macro definitions, updates Pipeline field types and PipelineTypeTraits template parameter
example/ck_tile/03_gemm/universal_gemm.cpp Updates pipeline comparisons to use enum values instead of macros
example/ck_tile/03_gemm/gemm_utils.hpp Removes macro definitions, updates Pipeline field types and PipelineTypeTraits template parameter
example/ck_tile/03_gemm/gemm_splitk_two_stage_reduce.cpp Updates pipeline comparisons to use enum values instead of macros
example/ck_tile/03_gemm/gemm_basic.cpp Updates pipeline comparisons to use enum values instead of macros

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


static constexpr bool DoubleSmemBuffer = false;
static constexpr bool DoubleSmemBuffer = false;
static constexpr ck_tile::GemmPipeline Pipeline = ck_tile::GemmPipeline::COMPUTE_V3;
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

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

The GemmConfigComputeV3_2 struct is missing the Pipeline field that other config structs inherit from GemmConfigBase. While line 87 adds it, the base class GemmConfigBase already defines Pipeline = COMPUTE_V3 (line 67), making this addition correct but potentially confusing. Verify that this struct intentionally overrides the base class default or if it should inherit it.

Suggested change
static constexpr ck_tile::GemmPipeline Pipeline = ck_tile::GemmPipeline::COMPUTE_V3;

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@AviralGoelAMD AviralGoelAMD left a comment

Choose a reason for hiding this comment

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

LGTM. As a sanity check, I also started CI on tile_engine.

amd-anclark
amd-anclark previously approved these changes Oct 31, 2025
cgmillette
cgmillette previously approved these changes Oct 31, 2025
Copy link
Collaborator

@cgmillette cgmillette left a comment

Choose a reason for hiding this comment

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

LGTM!

@ecamartins ecamartins dismissed stale reviews from cgmillette and amd-anclark via 1d74759 November 1, 2025 04:38
@ecamartins ecamartins force-pushed the emimarti/ck_tile/pipeline_enum branch from 563bc58 to 1d74759 Compare November 1, 2025 04:38
This change replaces pipeline macros like CK_TILE_PIPELINE_COMPUTE_V3,
CK_TILE_PIPELINE_MEMORY, etc in the CK Tile examples with a common enum
called GemmPipeline to reduce code duplication.
@ecamartins ecamartins force-pushed the emimarti/ck_tile/pipeline_enum branch from 1d74759 to 7579fa6 Compare November 2, 2025 22:17
@ecamartins ecamartins merged commit 2ec57a8 into develop Nov 3, 2025
49 checks passed
@ecamartins ecamartins deleted the emimarti/ck_tile/pipeline_enum branch November 3, 2025 16:35
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.

5 participants