-
Notifications
You must be signed in to change notification settings - Fork 286
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
[AddSeqMemPorts] Add hierpathop to verbatim, instead of raw instance path #7014
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, a lot cleaner 💯
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checkout HierPathCache
from FIRRTLAnnotationHelper.h
! 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks I missed HierPathCache
. Updated the PR to use the cache.
086cbbe
to
5f617f8
Compare
5f617f8
to
c6bb7fb
Compare
The annotation
AddSeqMemPortsFileAnnotation
causes the passAddSeqMemPort
to 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
HierPathOp
to 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
firtool
crash that is exposed when usingAddSeqMem
along 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.