Skip to content

[Bug Report] noc_inline_dw_write_set_state incorrect watcher assertion #20439

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.

Sign up for GitHub

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

Jump to bottom
Closed
nhuang-tt opened this issue Apr 9, 2025 · 1 comment · Fixed by #20519
Closed

[Bug Report] noc_inline_dw_write_set_state incorrect watcher assertion #20439

nhuang-tt opened this issue Apr 9, 2025 · 1 comment · Fixed by #20519
Assignees
@tt-dma
Labels
bug Something isn't working

Comments

@nhuang-tt
Copy link
Member

nhuang-tt commented Apr 9, 2025 •
edited by sync-by-unito bot
Loading

Describe the bug
Seen in tt_metal/fabric/hw/inc/tt_fabric.h:128

Device 0 active ethnet core(x= 0,y= 8) virtual(x=25,y=17): erisc using noc1 tried to unicast read 4 bytes to local L1[0x000000] from Unknown core w/ virtual coords (x=0,y=0) [addr=0xffb4a088] (NOC target address did not map to any known Tensix/Ethernet/DRAM/PCIE core)

To Reproduce
Run any program with 2D Push enabled and Watcher

export TT_METAL_WATCHER=1
export TT_METAL_WATCHER_NOINLINE=1
build/test/tt_metal/tt_fabric/fabric_unit_tests --gtest_filter=Fabric2DPushFixture.TestAsyncWriteAtomicInc

Expected behavior
Validation of NoC state should happen when kicking off the transaction, not when setting the state
@nhuang-tt nhuang-tt added the bug Something isn't working label Apr 9, 2025
@tt-dma
Copy link
Contributor

tt-dma commented Apr 10, 2025

Resolution is that we should remove the noc_sanitize check in this function - @nhuang-tt can you do this to unblock?

@nhuang-tt nhuang-tt mentioned this issue Apr 10, 2025
Merged
6 tasks
@nhuang-tt nhuang-tt linked a pull request Apr 10, 2025 that will close this issue
Sanitize NoC addr is not needed for set state #20519
Merged
6 tasks
github-merge-queue bot pushed a commit that referenced this issue Apr 10, 2025
@nhuang-tt
Sanitize NoC addr is not needed for set state (#20519)
2a1461d 
### Ticket
[Link to Github
Issue](#20439 (comment))

### Problem description
Watcher assertion is getting fired on this line

### What's changed
Sanitize NoC addr is not needed for set state because nothing is sent
out yet. The other "_set_state" function don't have any checks either.

### Checklist
- [ ] [All post
commit](https://github.com/tenstorrent/tt-metal/actions/workflows/all-post-commit-workflows.yaml)
CI passes
- [ ] [Blackhole Post
commit](https://github.com/tenstorrent/tt-metal/actions/workflows/blackhole-post-commit.yaml)
CI passes (if applicable)
- [ ] [Model
regression](https://github.com/tenstorrent/tt-metal/actions/workflows/perf-models.yaml)
CI passes (if applicable)
- [ ] [Device performance
regression](https://github.com/tenstorrent/tt-metal/actions/workflows/perf-device-models.yaml)
CI passes (if applicable)
- [ ] **(For models and ops writers)** Full [new models
tests](https://github.com/tenstorrent/tt-metal/actions/workflows/full-new-models-suite.yaml)
CI passes (if applicable)
- [ ] New/Existing tests provide coverage for changes
@github-project-automation github-project-automation bot moved this from 🆕 New to ✅ Done in External Requests and Reports Apr 10, 2025
ssinghalTT pushed a commit that referenced this issue Apr 23, 2025
@nhuang-tt @ssinghalTT
Sanitize NoC addr is not needed for set state (#20519)
8fc1995 
### Ticket
[Link to Github
Issue](#20439 (comment))

### Problem description
Watcher assertion is getting fired on this line

### What's changed
Sanitize NoC addr is not needed for set state because nothing is sent
out yet. The other "_set_state" function don't have any checks either.

### Checklist
- [ ] [All post
commit](https://github.com/tenstorrent/tt-metal/actions/workflows/all-post-commit-workflows.yaml)
CI passes
- [ ] [Blackhole Post
commit](https://github.com/tenstorrent/tt-metal/actions/workflows/blackhole-post-commit.yaml)
CI passes (if applicable)
- [ ] [Model
regression](https://github.com/tenstorrent/tt-metal/actions/workflows/perf-models.yaml)
CI passes (if applicable)
- [ ] [Device performance
regression](https://github.com/tenstorrent/tt-metal/actions/workflows/perf-device-models.yaml)
CI passes (if applicable)
- [ ] **(For models and ops writers)** Full [new models
tests](https://github.com/tenstorrent/tt-metal/actions/workflows/full-new-models-suite.yaml)
CI passes (if applicable)
- [ ] New/Existing tests provide coverage for changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tt-dma tt-dma

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Sanitize NoC addr is not needed for set state tenstorrent/tt-metal
2 participants
@tt-dma @nhuang-tt