[CK_BUILDER] Add conv factories for DeviceGroupedConvFwdMultipleABD_Xdl_CShuffle and DeviceGroupedConvFwdMultipleD_Wmma_CShuffle #3138
Conversation
…ts and device operations.
…ect load template parameter in CK.
…ralize-conv-factory
…emining-fwd-conv-device-ops
vpietila-amd
requested review from
ThomasNing,
afagaj,
andriy-ca,
aosewski,
asleepzzz,
bartekxk,
carlushuang,
coderfeli,
geyyer,
illsilin,
poyenc,
qianfengz and
tenpercent
as code owners
vpietila-amd
requested review from
cgmillette,
shumway and
vidyasagar-amd
as code owners
| FwdThreadBlock, | ||
| BlockGemmPipelineVersion::V1, | ||
| ConvFwdSpecialization::DEFAULT>(); | ||
| run_test_DeviceGroupedConvFwdMultipleABD_Xdl_CShuffle_V3<FwdConvSignature, |
There was a problem hiding this comment.
We're getting away from the builder concept here. Why should test code need to know about these old CK instances? Does this all break if we move to CK-tile? I know we need some internal translation in the library, but by the test layer this implemenation detail shouldn't be here.
|
|
||
| using namespace ck_tile::builder::test_utils; | ||
|
|
||
| namespace ck_tile::builder::testing { |
There was a problem hiding this comment.
Test code should not be in a testing namespace. The ck_test::builder::test namespace is for official CK library code that users can use to test our library. These tests should probably be in a simple anonymous namespace.
| { | ||
| constexpr auto gemm_spec = ALGORITHM.gemm_specialization; | ||
|
|
||
| if constexpr(gemm_spec == GemmSpecialization::Default) |
There was a problem hiding this comment.
switch-case is much more idomatic.
Also, we can use using enum aliases to clear out the repeated namespaces and enum class names.
It's OK to leave this for now, since it's easy to clean up later.
| BlockGemmPipelineVersion FwdPipelineVersion, | ||
| ConvFwdSpecialization FwdConvSpecialization> | ||
| constexpr void run_test() | ||
| constexpr void run_test_DeviceGroupedConvFwdMultipleABD_Xdl_CShuffle_V3() |
There was a problem hiding this comment.
The whole point of the builder is to hide these details like our different pipelines / device operator. We'll want to seamlessly transition to CK Tile. We need some glue code in the implementation, but I wouldn't expect details like this in the tests.
There was a problem hiding this comment.
I think moving forward like this is the best initial step, since we're learning and want to get to an end-to-end solution. We are building up testing and reflection support, so a lot of the testing will be simplified and replaced. There are also some idiomatic things like using switch-case we can add, but that's easy to fix later.
I think Adam wants to compare and contrast these concepts with the reflection code he's working on. We may refine them some and pick and choose the best names and heirarchy from both code trees (builder concepts and reflection). It would be good to capture the current state next week and start critiquing choices and seeing what we can simplify or where rough edges might be.
Proposed changes
DeviceGroupedConvFwdMultipleABD_Xdl_CShuffleandDeviceGroupedConvFwdMultipleD_Wmma_CShuffle.The builder and the convolution factories can now specify all template for the device operations. However, this PR doesn't yet have a mechanism to set default values for the optional template parameters via the builder. It will be introduced later as it requires a bit more design work.