#16434: DPRINT to read buffer once #16586
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tt-dma
requested review from
abhullar-tt,
pgkeller,
aliuTT,
tt-aho,
tt-asaigal and
ubcheema
as code owners
tt-dma
force-pushed
the
dma/16434_dprint_race
branch
from
ef88537 to
c6bfbf6
Compare
pgkeller
reviewed
Jan 17, 2025
| @@ -0,0 +1,27 @@ | |||
| // SPDX-FileCopyrightText: © 2023 Tenstorrent Inc. | |||
There was a problem hiding this comment.
copyright's on new files are out of date
pgkeller
approved these changes
Jan 17, 2025
Previously read twice (with the first read being eight bytes). Switching to single read appears to alleviate some specific race conditions where both dprint and risc-to-risc dependencies are present.
tt-dma
force-pushed
the
dma/16434_dprint_race
branch
from
889a498 to
83972d4
Compare
This was referenced Jan 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket
#16434
Also including a fix for #15694, as well as changing the default TT_METAL_DPRINT_CHIPS to all (an issue run into by model team on TG/TGG)
Problem description
@miawangTT hit what appears to be a very specific race condition w/ dprint + inter-risc dependencies resulting in some old data being read (specifically wpos was out of sync with print data). Although I have so far been unable to determine exact location where this race is coming from, switching dprint from a double read to single read does seem to fix the cases where I was able to reproduce this.
I do think single read is correct over what we had before, since we were using wpos/rpos from the first read and data from the second read.
What's changed
Switch DPRINT from reading buffer twice (once first 8 bytes, then conditional second) to a single read. Wasn't sure if the previous method even gave any performance benefit, but it should be fine either way since this is a debug only path.
Checklist