[AddSeqMemPorts] Add hierpathop to verbatim, instead of raw instance path #7014
Conversation
This file contains hidden or 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
fabianschuiki
approved these changes
May 9, 2024
Comment on lines
179
to
191
| // CHECK-SAME{LITERAL}: 1 -> {{0}}.{{2}}.{{3}} | ||
| // CHECK-SAME{LITERAL}: 2 -> {{0}}.{{4}} | ||
| // CHECK-SAME: {symbols = [@DUT, #hw.innerNameRef<@DUT::@[[MWRITE_EXT]]>, #hw.innerNameRef<@DUT::@[[CHILD]]>, #hw.innerNameRef<@Child::@[[CHILD_MWRITE_EXT]]>, #hw.innerNameRef<@DUT::@[[MWRITE_EXT_0]]>]} | ||
| // CHECK-SAME{LITERAL}: 1 -> {{0}}.{{2}} | ||
| // CHECK-SAME{LITERAL}: 2 -> {{0}}.{{3}} | ||
| // CHECK-SAME: {symbols = [@DUT, @memNLA, @memNLA_0, @memNLA_1]} |
There was a problem hiding this comment.
Nice, a lot cleaner 💯
dtzSiFive
reviewed
May 10, 2024
| // Now add the nonlocal annotation to the leaf instance. | ||
| auto *leafInstance = innerRefToInstanceMap[instancePath.front()]; | ||
| // TODO: Check if we can reuse HierPathOp, if it already exists! | ||
| AnnotationSet annos(leafInstance); |
There was a problem hiding this comment.
Checkout HierPathCache from FIRRTLAnnotationHelper.h! 😁
There was a problem hiding this comment.
Awesome, thanks I missed HierPathCache. Updated the PR to use the cache.
086cbbe to
5f617f8
Compare
5f617f8 to
c6bb7fb
Compare
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.
The annotation
AddSeqMemPortsFileAnnotationcauses the passAddSeqMemPortto add verbatim ops for the SRAM metadata file.The file lists each SRAM and provides the mapping to
where it is in the hierarchy, and gives its IO prefix at the DUT top level.
This PR updates the pass to use
HierPathOpto print the SRAM instance path,instead of the raw list of symbol references.
This is required to ensure that the instance path is valid, even if the hierarchy
is updated by following passes.
This fixes a
firtoolcrash that is exposed when usingAddSeqMemalong withExtractSeqMem. Which is due to invalid symbol references in thevarbatim, afterthe hierarchy was updated. Even though, this particular use case is invalid, as these
two SRAM annotations cannot be used together, it can still be triggered by other
transformations.
Note: This metadata will soon be moved to OM, in a followup PR.