Addressing (Post Merge) code review comments for PR 1845

[kylasa]

Proposed changes

Following PR is used to address post-merge comments for PR 1845 (enabling datatypes for GEMM).

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

Discussion

If this is a relatively large or complex change, feel free to start a discussion by explaining why you chose the solution you did and what alternatives you considered

[ThomasNing]

We could create a function "template <typename LayoutA, typename LayoutB, typename LayoutC>
int run_gemm_example_data_type_switch(int argc,
char* argv[],
const std::string& data_type)
{
if(data_type == "fp16")
{
// Using ck_tile::half_t
return run_gemm_example_with_layouts<ck_tile::half_t, LayoutA, LayoutB, LayoutC>(
argc, argv);
}
}" and call it with "return run_gemm_example_data_type_switch<Row, Row, Row>(argc, argv, data_type);" in the run_gemm_example function.

[aosewski]

@kylasa Please refactor data type selection logic. You could do this in similar way as Thomas pointed. In general IMHO you could achieve this in two ways, where each is equally good. You have to add one another level of function call. Lets say you first differentiate only data type, then you should first call sth like: run_gemm_example_with_dtype with concrete data type used as a tparam and layouts passed as strings args (you'd have to make if...else ladder to select the type) - this is first level. In second level in run_gemm_example_with_dtype function body you'd just create the another if...else ladder but this time you'd differentiate only the selected layout and call final stage function which would proceed with rest of the regular logic.

In general as I said you could do this both ways, either first differentiate data type, then layout, or in reverse order.

Other important thing is @kylasa please make sure that each enabled configuration of data type & layout is producing correct results. If any of them happen to not work, then please comment it with a TODO note.

[kylasa]

Thanks for your suggestions @ThomasNing and @aosewski. Code changed accordingly.

[ThomasNing]

@kylasa Please also add the test cases for different data types in the test/ck_tile/gemm/test_gemm_pipeline.cpp.

[ThomasNing]

@illsilin As we discussed yesterday, you could try running the benchmark and smoke test for different datatypes on this PR to see if we will put CK TILE GEMM as our CI daily running. The benchmark and smoke test scripts are already there.

[aosewski]

@illsilin As we discussed yesterday, you could try running the benchmark and smoke test for different datatypes on this PR to see if we will put CK TILE GEMM as our CI daily running. The benchmark and smoke test scripts are already there.

@ThomasNing I believe that in regular CI run we shouldn't run both benchmark & smoke tests - those kind of scripts should be run at much less frequency like daily or weekly.. We have unit-tests for CK Tile GEMM which are already enabled as I asked about that Illia some time ago.

[ThomasNing]

@aosewski Yes, Illia said he will test the running time for the benchmark to decide what's the frequency we should run those tests (in one/two days period). We are just using this PR to add the test script for datatypes except f16.