-
Notifications
You must be signed in to change notification settings - Fork 651
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
Add support for NoC based interconnect with Constellation #1224
Conversation
# Constellation can run without espresso, but this improves | ||
# elaboration time drastically | ||
pushd $REMOTE_CHIPYARD_DIR/generators/constellation | ||
scripts/install-espresso.sh $RISCV |
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 is espresso? Is this like graalvm?
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.
https://github.com/chipsalliance/espresso .
Espresso is a logic minimizer that Constellation calls (through chisel3) to generate routing tables.
If espresso
is not installed, the generator falls back on a slower implementation.
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 inclined to just add this installation in build-toolchains-extra.sh
and add a flag to skip this if users don't want 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.
I'm going to leave it in like this for now, to avoid more churn on the install scripts.
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.
Sure. Can you make an issue so this is noted and we can put it into the next release? I get the feeling that majority of people won't have this installed and will complain about slowness
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.
Sure, I added an issue. There has been discussion of packaging espresso as a Scala library so it can just be added as a SBT dependency to Chisel, but I don't think there's been any progress on that.
Also, the performance improvement is only apparent for large many core designs, which are still slow to elaborate due to the cost of elaborating many cores.
ae534f2
to
746d28a
Compare
746d28a
to
d554c28
Compare
This also adds a new |
aee1ddf
to
07cad27
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! good stuff!
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.
Quick review without viewing the docs. I'll trust Sagar on the docs front.
new constellation.soc.WithSbusNoC(constellation.protocol.TLNoCParams( | ||
constellation.protocol.DiplomaticNetworkNodeMapping( | ||
inNodeMapping = ListMap( | ||
"Core 0" -> 1, "Core 1" -> 2, "Core 2" -> 4 , "Core 3" -> 7, |
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.
How are these strings determined?
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.
Currently its ad-hoc based on how the TL agents are diplomatically instantiated, whether or not an explicit name is provided.
This is admittedly very messy currently, the names often do not reflect what the device is.
This should be cleaned up in the future.
@@ -23,7 +23,7 @@ class TinyRocketConfig extends Config( | |||
|
|||
// DOC include start: FFTRocketConfig | |||
class FFTRocketConfig extends Config( | |||
new fftgenerator.WithFFTGenerator(baseAddr=0x2000, numPoints=8, width=16, decPt=8) ++ // add 8-point mmio fft at 0x2000 with 16bit fixed-point numbers. | |||
new fftgenerator.WithFFTGenerator(numPoints=8, width=16, decPt=8) ++ // add 8-point mmio fft at 0x2000 with 16bit fixed-point numbers. |
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.
Comment needs to be updated. However, why was this address removed in favor of it being @ 0x2400
?
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.
Making all the MMIO devices use different addresses makes it easier to generate and test 1 config that contains many devices.
I updated the SW tests as well for these.
@@ -13,7 +13,7 @@ import freechips.rocketchip.util.UIntIsOneOf | |||
|
|||
// DOC include start: GCD params | |||
case class GCDParams( | |||
address: BigInt = 0x2000, | |||
address: BigInt = 0x1000, |
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.
Again, why is this address moved?
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 you can comment on this for all other MMIO addresses moving?
MODEL ?= TestHarness | ||
VLOG_MODEL ?= TestHarness | ||
MODEL_PACKAGE ?= constellation.test | ||
CONFIG ?= TestConfig00 |
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.
00
?
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.
As opposed to TestConfig01
, TestConfig02
, etc.
Added support for NoC-based interconnect via Constellation.
Related PRs / Issues:
Type of change:
Impact:
Contributor Checklist:
main
as the base branch?changelog:<topic>
label?changelog:
label?.conda-lock.yml
file if you updated the conda requirements file?Please Backport
?