-
Notifications
You must be signed in to change notification settings - Fork 281
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
[FIRRTL] Use symbols in ResolveTraces verbatims #5752
Conversation
f8f1acb
to
ddfa83b
Compare
CC: @sequencer: Without this, on CIRCT 1.48 or earlier, you could get incorrect names (e.g., colliding names and some names that are wrong in Chisel tests). Without this, on CIRCT top-of-tree, you can get totally wrong names that point to nothing in the design. 😬 This will be in CIRCT 1.49 (probably next week). |
Thanks for mention! I'll bump to 1.49 when it comes. |
ddfa83b
to
1c63a13
Compare
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
Add unused "<<" operators for AnnoPathValue, OpAnnoTarget, PortAnnoTarget, and AnnoTarget. These facilitate debugging in FIRRTL passes. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Change the ResolveTraces pass to use symbols instead of hard-coded names. Previously, the pass used hard-coded names derived from the hierpath op symbols (which was mega-jank! I'm sorry!). Correct all this by using symbols with substitutions. Update the end-to-end test to use a test case which uncovered this error (because it uses an instance path). Update the pass unit tests to use the new format. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
1c63a13
to
6f9c745
Compare
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.
Thanks! Sorry for not getting to review earlier, good study of what works (and is painful/could-be-better) re:inner symbols. Left some comments, FWIW.
newTarget.append(">"); | ||
auto innerSym = | ||
TypeSwitch<AnnoTarget, hw::InnerSymAttr>(path.ref) | ||
.Case<PortAnnoTarget>([&](PortAnnoTarget portTarget) { |
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.
Is it known/required these already have inner symbols on them? (I'm guessing "yes")?
(and is this why not using, e.g., AnnoTarget::getInnerSym
?)
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.
That would be better!
|
||
auto type = dyn_cast<FIRRTLBaseType>(path.ref.getType()); | ||
assert(type && "expected a FIRRTLBaseType"); | ||
auto targetFieldID = path.fieldIdx; |
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.
Note-to-self: Revisit this to actually put the symbol on the target wanting to be tracked (+field-id), instead of on root + generating indexing suffix that we hope will be valid (we do this elsewhere).
This is the sort of thing we do presently, until ... well pipeline is fixed to handle all of this.
hw::HierPathOp path = nlaTable->getNLA(nla.getAttr()); | ||
for (auto part : path.getNamepath().getValue().drop_back()) { | ||
auto inst = cast<hw::InnerRefAttr>(part); | ||
instances.push_back(dyn_cast<InstanceOp>( |
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.
Ah, need to make it easier to use IRN -- 100% should be easy to ask for resolution of an inner ref. Anyway, thanks for working through the current situation (including re:other comments).
@dtzSiFive comments addressed in: f69e974 Thanks! |
Change the ResolveTraces pass to use symbols instead of hard-coded names. Previously, the pass used hard-coded names derived from the hierpath op symbols (which was mega-jank! I'm sorry!). Correct all this by using symbols with substitutions.
Update the end-to-end test to use a test case which uncovered this error (because it uses an instance path). Update the pass unit tests to use the new format.