Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#15450: Remove default values from circular buffer parameters in LLK compute APIs: Test Kernels #16613

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

atatuzunerTT
Copy link
Contributor

@atatuzunerTT atatuzunerTT commented Jan 10, 2025

Ticket

Link to Github Issue

Problem description

Default values for circular buffer arguments in the LLK compute API can cause errors. Forgetting to set these arguments explicitly may lead to errors due to wrong cb usage. This PR is specific to the changes in the internal test kernel APIs:

  • ./tests/tt_metal/tt_metal/test_kernels/compute/reduce_h.cpp
  • ./tests/tt_metal/tt_metal/test_kernels/compute/reduce_hw.cpp
  • ./tests/tt_metal/tt_metal/test_kernels/compute/reduce_w.cpp
  • ./tests/tt_metal/tt_metal/test_kernels/compute/unpack_tilizeA_B.cpp

What's changed

Default values for the circular buffer parameters have been removed from functions within these files. The call chains invoking these functions have been updated to contain explicit arguments for these parameters.

Checklist

  • Post commit CI passes
  • Blackhole Post commit (if applicable)
  • Model regression CI testing passes (if applicable)
  • Device performance regression CI testing passes (if applicable)
  • (For models and ops writers) Full new models tests passes
  • New/Existing tests provide coverage for changes

@atatuzunerTT atatuzunerTT marked this pull request as ready for review January 14, 2025 14:14
@@ -44,7 +44,7 @@ void MAIN {
constexpr uint32_t Wt = get_compile_time_arg_val(1);
constexpr uint32_t NC = get_compile_time_arg_val(2);
constexpr bool at_start = get_compile_time_arg_val(3);
dummy_init<at_start>(tt::CBIndex::c_0, tt::CBIndex::c_2);
dummy_init<at_start>(tt::CBIndex::c_0, tt::CBIndex::c_2, tt::CBIndex::c_16);
#ifndef SHORT_INIT
reduce_init<at_start>(tt::CBIndex::c_0, tt::CBIndex::c_2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Will output cb to reduce_init be added in a separate pr addressing reduce_init default arg removal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, here is the PR for that

@atatuzunerTT atatuzunerTT force-pushed the atuzuner/remove_default_values_test_kernels branch from 7d471be to 3798f1c Compare January 16, 2025 22:09
@atatuzunerTT atatuzunerTT merged commit 8979677 into main Jan 16, 2025
9 checks passed
@atatuzunerTT atatuzunerTT deleted the atuzuner/remove_default_values_test_kernels branch January 16, 2025 22:12
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.

3 participants