-
Notifications
You must be signed in to change notification settings - Fork 298
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][CreateSifiveMetadata] Use symbols for memory metadata #5482
Conversation
// CHECK: sv.verbatim "[\0A {\0A \22module_name\22: \22 | ||
// CHECK-SAME: {{0}} | ||
// CHECK-SAME: \22,\0A \22depth\22: 16,\0A \22width\22: 8,\0A \22masked\22: false,\0A \22read\22: 1,\0A \22write\22: 0,\0A \22readwrite\22: 1,\0A \22extra_ports\22: [],\0A \22 | ||
// CHECK-SAME: hierarchy\22: [\0A \22{{[{][{]3[}][}]}}.{{[{][{]4[}][}]}}.memory_ext |
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.
Suggestion: Can this be simplified/more readable with CHECK-SAME{LITERAL}:
instead?
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.
Yes, thanks for reminding. Updated with it.
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
Can you give this a spin on an internal design and check the metadata before/after to make sure things look good?
auto dutVerbatimOp = builder.create<sv::VerbatimOp>( | ||
builder.getUnknownLoc(), dutJsonBuffer, ValueRange(), | ||
builder.getArrayAttr(jsonSymbols)); |
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.
Nit: the PR could save on the two builder.getUnknownLoc()
by using an ImplicitLocOpBuilder
instead of an OpBuilder
.
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 for catching this. Fixed.
0faf261
to
c933dd9
Compare
Thanks for review. I tested on few internal designs, looks good. Also verified that this fixes the issue with ForceNameAnnotation on the internal design. |
Use symbols to emit memory metadata, this ensures that the instance names respect any renaming due to
ForceNameAnnotation
. Since memory instance cannot be renamed, use it as verbatim, this ensures that the pre-extract paths for seq mems are used.Fixes #5469
For reference, the previous change related to ExtractSeqMems, #3123