Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support for verilog memory loading. #840
Support for verilog memory loading. #840
Changes from 20 commits
521d77e
76a9b91
118bb7d
6cc1353
0e5cc30
4f55af1
14cea70
28580f9
24571c7
8a3bd21
8df88d9
66fc4ca
88bf995
754eb7d
0712c3a
0b15393
09896d0
cbf1d49
5a1161c
a6d8ee5
38a1343
60e9627
ffb8d2b
9d09b52
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Things in package
chisel3.util
should try to be like what any library would look like so it should not reach intochisel3.core
(import stuff directly fromchisel3_
andchisel3.experimental._
instead).Also it should avoid use of
chisel3.internal
, and avoidprivate[chisel3]
stuff (like the Builder). I think the only use of Builder is for a warning below that should probably just be an exception, or better yet, a requirement below.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.
Done, I had to add experimental.annotate and chisel3.InstanceId to the chisel package. But I think that is correct as these are part of the public API
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: should this PR follow the style guide suggestion of explicit imports for anything not
import chisel3._
?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: I've seen Jack using
import firrtl.{ir => fir}
which I kind of liked. Perhaps consider that and you can avoid the need to excludeModule
and then use it's full package path below.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.
Which style guide are you referring to here?
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.
Sorry, didn't respond... The one sitting in doc/style.md.
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 there any reason to make the target
InstanceId
or should it be the more preciseMemBase
as inloadMemoryFromFile.apply
below?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.
I did not have any luck changing this. Could not get it to compile despite a number of attempts to work with MemBase type parameters
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.
Here's a diff that accomplishes what I would like to see
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 supposed to be an error if it gets to post-elaboration
toFirrtl
(LoadMemoryAnnotation
is supposed to be treating this as an exception). I'd suggest either not emitting a warning or emitting an error. I'm tempted to go with the former as I don't think there's significant time savings in failing during elaboration vs. post elaboration. @jackkoenig may be able to comment on the utility of this type of elaboration failure for large designs.Note: the supposed to be qualifier above is due to me looking at how
LoadMemoryAnnotation
works. However, an empty filename isn't resulting inNil
as I think it's supposed to. See: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 should probably just be:
require(fileName.nonEmpty, "LoadMemory from file annotations file empty file name")
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.
I took the easy way out here and Changed to Builder.error. I don't think it hurts to do it sooner. I don't think it's a particularly likely case anyway.
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.
I made it an exception, mostly to get rid of the Builder reference.
This is a relatively unlikely (and clearly wrong) edge case. I'd rather keep the fixes simple as possible.
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.
I expected that this would be in the FIRRTL tree as opposed to Chisel. Not that big of a deal, but that seems more logical to me for things which are FIRRTL-proper as opposed to libraries. What do you think?
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.
I'm kind of in the opposite camp. I am more interested in examples of how to write things that are more like external libraries that do not require changes to the underlying firrtl code. This was a bit of a fail in that it required changes to firrtl to support generating modules bound to other module, but this should lead to exposing bound modules as an API for library developers. So hopefully this is sufficient justification for where this is.
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.
I see what you're saying. Nothing like that exists in the Chisel tree (as far as I'm aware). External libraries show it, but that's all.
I guess then there's the reverse question of why
LoadMemoryAnnotation
andMemoryLoadFileType
aren't in Chisel?I think this occurred to me as reading the code I was bouncing back and forth between the repos: a
chisel3.util.experimental.ChiselLoadMemoryAnnotation
generates afirrtl.LoadMemoryAnnotation
which is processed by achisel3.util.experimental.LoadMemoryTransform
which interacts with the thefirrtl.VerilogEmitter
.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.
I am trying to write as much of this stuff as a library without changing firrtl.
It's true that for this project firrtl did get changed but that was because we needed some low level support for access to verilog module headers to match signatures for module binding, but I think that's justifiable because it is the beginning of an API for other work with verilog binding.
Bottom line, I'd prefer things to stay where they are now.
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.
Got it. Yeah, not the low level stuff. I'm only narrowly talking about the contents of FIRRTL's LoadMemoryAnnotation.scala. Moving that (and only that) over here would make this entirely encapsulated in Chisel. Maybe a subsequent PR? (Or, I can slam a PR through right now if that's something you want in this PR). No reason to hold this up for that, though.
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.
I think that this is equivalent to the
serialize
method ofComponentName
.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.
It is, I'm going to leave this for now pending a more general discussion of what a
ComponentName
actually is.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] This doesn't have the possibility of a name collision, but you could consider using a
groupBy
(and modifying the code below`).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 only issue with groupBy is that it returns a Seq, this could still be simpler as:
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, @jackkoenig. 🏌️♂️ This looks fine.
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 test :)
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
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.
I prefer to prevent IntelliJ from getting to commit to the repo... Is it okay to remove this?
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.
I guess I'm in the other camp, where I really like to have my files "style clean", with annotations like this for when I intentionally violate the rules. I have changed this to
//scalastyle:off method.length
Which is more specific and sufficient to make the file get a ✅
I hope that's ok.
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.
Scalastyle comments lgtm.
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.
Remove?
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.
done
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.
I'd like to see this PR add some end-to-end testing of memories. I.e., add some resources of readmemh/readmemb format and then make sure that the data matches up.
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.
I have end to end testing in an as-yet-unpushed PR on chisel-testers because of all the heavy lifting that is done there to invoke verilator with all the right arguments and file names. Because of the testers2 project Richard is working on I'm not really sure where that is all going to live come 3.2, but I think it is as yet un-PR'd chisel3
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.
Sounds great. I should have expected that you had something lined up...