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
[codegen] Emit literal identifiers for numeric ids #3374
Conversation
d9de057
to
16dcbd7
Compare
b0a8977
to
31e9a23
Compare
cb19f5e
to
f8c7fae
Compare
31e9a23
to
c228de4
Compare
f8c7fae
to
72f4f26
Compare
88f5517
to
f791e45
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.
Generally LGTM but question about verification statements
readwriters.foreach { r => b ++= "readwriter => "; b ++= r; newLineAndIndent(1) } | ||
readers.foreach { r => b ++= "reader => "; b ++= legalize(r); newLineAndIndent(1) } | ||
writers.foreach { w => b ++= "writer => "; b ++= legalize(w); newLineAndIndent(1) } | ||
readwriters.foreach { r => b ++= "readwriter => "; b ++= legalize(r); newLineAndIndent(1) } | ||
b ++= "read-under-write => "; b ++= readUnderWrite.toString | ||
case Attach(info, exprs) => | ||
// exprs should never be empty since the attach statement takes *at least* two signals according to the spec |
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.
What about verification Statements? They have labels which are names in the sense that they can be annotated and they should correspond to labels in the Verilog.
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.
Good catch. I had considered this and thought it wasn't necessary. However, it is. I added a commit that makes these literal identifiers and adds checks that it works here: 9587c6c
Change the FIRRTL serializer to use literal identifiers if any identifiers begin with a leading digit. This is a FIRRTL 3.0.0 feature that simplifies parsing. This enables literal identifier emission anywhere. However, this code path is only reachable via MixedVec due to mangling of numeric names to add a leading underscore in all other circumstances. In the future, this commit will enable removing this restriction and switching to literal identifiers when this happens. No changes are made to annotation targets. E.g., the literal identifier, "wire `42`" in circuit "Foo" and module "Bar" is still referred to by a local target "~Foo|Bar>42". This is intentional (for now). Targets don't have parsing ambiguity like FIRRTL text and the name of this wire is still "42" not "`42`". It follows that the storage of this name in FIRRTL IR is not changed. This is the same way that this is handled in CIRCT. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
f791e45
to
9587c6c
Compare
Change the FIRRTL serializer to use literal identifiers if any identifiers begin with a leading digit. This is a FIRRTL 3.0.0 feature that simplifies parsing.
This enables literal identifier emission anywhere. However, this code path is only reachable via MixedVec due to mangling of numeric names to add a leading underscore in all other circumstances. In the future, this commit will enable removing this restriction and switching to literal identifiers when this happens.
No changes are made to annotation targets. E.g., the literal identifier, "wire
42
" in circuit "Foo" and module "Bar" is still referred to by a local target "~Foo|Bar>42". This is intentional (for now). Targets don't have parsing ambiguity like FIRRTL text and the name of this wire is still "42" not "42
". It follows that the storage of this name in FIRRTL IR is not changed. This is the same way that this is handled in CIRCT.Release Notes
Use FIRRTL 3.0.0 emission of literal identifiers when a Chisel name begins with a digit.
Notes
This PR is stacked on #3188 (and should be merged after it).
Example
With this commit, this will produce the following FIRRTL and Verilog:
With further changes that go beyond this PR< this should emit as: