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

Removing skip_for_blackhole and fix some minor issues for blackhole conv2d #17222

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

skrsticTT
Copy link
Contributor

Issues

#17221
#17216

Problem description

Enable conv2d blackhole tests - to begin with, remove the skips in tests/ttnn/unit_tests/operations/test_new_conv2d.py and provide a report about state of tests. Fix minor bugs described in issues.

What's changed

  • Removed @skip_for_blackhole() for tests that already run on wormhole_b0
  • Removed skip for tests that needs 8x8 grid for blackhole - it caused OOM for 8x7 grid on wormhole_b0, it should not be case on blackhole
  • Changed alignment of an output tensor in calculate_L1_usage function
  • Removed fatals for num_cores in width sharded factory

State of blackhole tests after this changes

repro: pytest tests/ttnn/unit_tests/operations/test_new_conv2d.py

PASSED: 2244
FAILED: 65
97% pass rate

Of the tests that fail:
47 of them fail because of PCC error
18 of them fail because of an assert conv2d_op_width_sharded_program_factory.cpp
(these problems are going to be tracked in proper issues)

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

@skrsticTT skrsticTT self-assigned this Jan 28, 2025
@skrsticTT skrsticTT requested a review from a team as a code owner January 28, 2025 09:50
@@ -578,7 +579,7 @@ def test_conv_ws(
auto_shard,
tilized_input,
):
if device.core_grid.y != 8:
if device.core_grid.y != 8 and is_wormhole_b0():
pytest.skip("Needs 8x8 Grid")
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment doesn't make sense anymore

@@ -983,6 +983,8 @@ conv_op_l1_usage conv2d::calculate_L1_usage(
(per_core_out_matrix_height_ntiles + act_block_h_ntiles - 1) / act_block_h_ntiles;
uint32_t out_block_h_ntiles_padded = num_blocks_act_h_per_core * act_block_h_ntiles;

uint32_t alignment_bytes = arch == tt::ARCH::BLACKHOLE ? 64 : 32;
Copy link
Contributor

Choose a reason for hiding this comment

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

Get l1 aligment from HAL

Copy link
Contributor

@pavlejosipovic pavlejosipovic left a comment

Choose a reason for hiding this comment

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

You will need to disable shallow convs for BH in auto shard codepath.

Copy link
Contributor

@pavlejosipovic pavlejosipovic left a comment

Choose a reason for hiding this comment

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

18 of them fail because of an assert conv2d_op_width_sharded_program_factory.cpp specify which assert is problematic and post new issue here.

@skrsticTT skrsticTT force-pushed the skrstic/enable-bh-tests branch from c5b8abc to 1733874 Compare January 29, 2025 18:06
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