-
Notifications
You must be signed in to change notification settings - Fork 177
Create annotation to allow inline readmem in Verilog #2107
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.
This looks great @carlosedp !
Extending the MemoryInitValue
and MemoryInitAnnotation
is definitely the way to go.
All I am asking you to do is move the code that generates the actual Verilog into the Verilog emitter (see my comments below). This is in order to keep the other files more generic so that they could be reuse by a (hypothetical) VHDL or yosys RTLIR emitter.
06315dd
to
b701ec4
Compare
@ekiwi Thanks for the great feedback, makes total sense having code related to Verilog in one place. |
Looks good (the reason CI is failing is that you need to run For testing, I would suggest that you look at the following file and try to add some tests for your new Annotation to it: https://github.com/chipsalliance/firrtl/blob/master/src/test/scala/firrtlTests/MemoryInitSpec.scala |
b701ec4
to
31d4849
Compare
Thanks Kevin, I've fixed the formatting and also added some tests to the new annotation. |
9ab5d65
to
299fdaf
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.
Looks like a great improvement.
I have a minor request that the error is promoted to a construction-time error.
if (filename.trim().isEmpty()) { | ||
throw EmitterException(s"empty filename not allowed in MemoryFileInlineAnnotation") | ||
} |
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.
This very late runtime check (running during Verilog emission) should be promoted to a construction-time requirement on the annotation. That way, you'll get an error as soon as you fail the requirement specified here (like after Chisel elaboration) as opposed to having to wait for the full FIRRTL compile.
(Tangentially, other code here is doing late checks. No reason to follow that pattern here if the error can be produced earlier.)
assertThrows[EmitterException] { | ||
compile(Seq(MemoryFileInlineAnnotation(mRef, filename = ""))) | ||
} |
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.
The exception type will change here with the change to a require
that runs during object creation. (No need to then do compile
after that fix.)
Once @seldridge's suggestion have been addressed, I would be happy to get this in. |
299fdaf
to
f7936e3
Compare
Addressed @seldridge comments on latest push. The validation is done on MemoryFileInlineAnnotation class instead of the VerilogEmitter. |
This PR adds a new annotation allowing inline loading for memory files in Verilog code.
f7936e3
to
b3d5edc
Compare
Just pushed as suggested! Thanks for pointing it out :) |
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! This looks great. 🚢
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.
This is great, thanks @carlosedp for your high quality contribution!
This PR adds a new annotation allowing inline loading for memory files in Verilog code. (cherry picked from commit efdefde)
Thanks for all the patience and guidance on this @ekiwi @seldridge @jackkoenig! |
This PR adds a new annotation allowing inline loading for memory files in Verilog code. (cherry picked from commit efdefde) Co-authored-by: Carlos Eduardo <[email protected]>
This PR adds a new annotation allowing inline loading for memory files in Verilog code.
Below a sample code:
Generated verilog:
Generated Verilog file was synthesized on Yosys for testing. Output below:
Details
Contributor Checklist
Type of Improvement
API Impact
No impact on previous methods or API, adds new annotation.
Backend Code Generation Impact
This annotation adds
readmem[hb]
statements to generated Verilog.Desired Merge Strategy
Release Notes
(addition) Added MemoryFileInlineAnnotation to allow loading hex and bin memory files inline in Verilog emitter(#2107)
Reviewer Checklist (only modified by reviewer)
Please Merge
?