Skip to content

Conversation

@Snektron
Copy link
Contributor

Proposed changes

In #3381, reference algorithms were added to CK Tile. But it turns out that there already was a reference implementation in old CK. This PR replaces the reference conv implementation used by CK-Builder by the one from CK for the following reasons:

  • The CK implementation has more tests.
  • The CK implementation supports different layouts.
  • The CK implementation supports non-passthrough element-wise operations.

This is still not a complete reference implementation though (no multi A/B/D support), but we can add that when required.

Details

This mainly just replaces the implementation that is called by the reference convolution factory, but there are a few extra details in this PR that should be noted:

  • I removed most of the "old CK api" from the reference convolution factory. I don't think there is any value if the main way to invoke these kernels is going to be to ckt::run() anyway. I also removed the corresponding tests.
  • I replaced the test_reference_execution comparison kernel with the old CK one too. Perhaps there is some value in comparing the two implementations, if we want to keep the old one around.
  • I changed the interface of ConvTensorLayouts to not require the SPATIAL_DIM. Its passed via SIGNATURE anyway, so it doesn't make sense to pass it extra. This also cleans up some of the use cases of that type.

Open Questions

  • Should we keep the CK Tile reference implementation? Its used in an example, so I guess its used outside of Builder.
  • Should we add a CPU reference implementation to test the GPU reference implementation against?

This information is already in the SIGNATURE, so its pointless to pass it
separately. This streamlines the interface of those functions a bit. Also
touches up the style of those files in general.
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 replaces the CK Tile reference convolution implementation with the more comprehensive reference implementation from old CK. The change improves test coverage and adds support for different layouts and non-passthrough elementwise operations.

Changes:

  • Replaced CK Tile reference implementation with old CK reference implementation in the reference factory
  • Removed "old CK API" methods (MakeArgumentPointer/MakeInvokerPointer) and associated tests
  • Simplified ConvTensorLayouts interface to remove redundant SPATIAL_DIM template parameter

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test_reference_instance_traits.cpp Removed debug console output statements
test_reference_execution.cpp Updated tests to use new reference implementation API, removed old CK interface tests
unit_conv_tensor_layout.cpp Removed SPATIAL_DIM template parameter from ConvTensorLayouts usage
conv_fwd.hpp Updated to use simplified ConvTensorLayouts interface
reference_factory.hpp Replaced implementation to call old CK reference functions, removed old API methods
reference_common.hpp File deleted as it's no longer needed
conv_tile_tensor_layout.hpp Updated template parameters to use SCREAMING_SNAKE_CASE, removed SPATIAL_DIM parameter
conv_tensor_layout.hpp Updated template parameters to use SCREAMING_SNAKE_CASE, removed SPATIAL_DIM parameter
Multiple factory files Updated to use simplified ConvTensorLayouts interface

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

@Snektron Snektron force-pushed the rvoetter/conv-testing-reference-old-ck branch from 4dfb87d to 04bcd4b Compare January 19, 2026 12:54
The old ck implementation is more featureful and better tested.
This strips out the ck-tile gpu reference implementation completely.
- Remove unneccesary messages
- Replace EXPECT_TRUE(true) with EXPECT_NO_THROW()
@Snektron Snektron force-pushed the rvoetter/conv-testing-reference-old-ck branch from 04bcd4b to a2130d8 Compare January 20, 2026 09:21
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