Skip to content

Conversation

@kabrahamAMD
Copy link
Contributor

Added constraints for (c++20 concepts) for relations between thread block size, m/n_per_xdl and warp size for convolutions built with ck builder. In this process, it was also discovered that the example in test_conv_description.cpp was erronous and m/n_xdl_per_wave should be increased from 4 to 8.

shumway
shumway previously approved these changes Nov 26, 2025
Copy link
Collaborator

@shumway shumway left a comment

Choose a reason for hiding this comment

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

This looks great!
That change to test_conv_description.cpp may break on gfx950, if so you may have to experiment a bit more with valid block tiling in that test.

@shumway shumway changed the title [CK Builder] Add constraints for block tiling [CK_Builder] Add constraints for block tiling Nov 26, 2025
(Value[2] >= 0 && Value[2] < 3));
};

// Limits for tile sizes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this universal for all architectures (it looks like a generic mathematical condition)? Maybe add a short explanation why the conditions logically obvious or if there is a technical reference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to break the constraints into more fine granular pieces to get a better compile-time error? I think in the current implementation it would be difficult to figure out which condition is violated.

@shumway shumway changed the title [CK_Builder] Add constraints for block tiling [CK_BUILDER] Add constraints for block tiling Nov 27, 2025
@vpietila-amd
Copy link
Contributor

The limits for tiles size are very useful since incorrect combinations of tile sizes are probably the biggest sources of errors when adding new kernel instances. However, I was under the impression that we wanted to collect the constraints into a single location where both the builder and the CK Tile implementation could use them. Maintaining a separate set of rules/constraints for the builder is a bit tricky since we don't have a good way of ensuring that they remain in sync with the implementation.

@shumway
Copy link
Collaborator

shumway commented Dec 2, 2025

Looks like there's a failure on gfx942 with this test: experimental/builder/test/conv/test_ckb_conv_fwd_1d_i8.cpp

Those i8 tests seem to be the most fragile.

@kabrahamAMD kabrahamAMD force-pushed the kabraham/builder_add_constraints branch from 6fa4948 to 4c9f3f3 Compare December 3, 2025 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants