-
Notifications
You must be signed in to change notification settings - Fork 813
Modularize out most submodules #2287
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
Conversation
2cf2e18 to
866aea0
Compare
866aea0 to
95f53e7
Compare
76580b2 to
4929918
Compare
4929918 to
e056889
Compare
|
From Codex: Review |
|
Also trying this. @codex review |
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.
Codex Review: Here are some suggestions.
chipyard/generators/firechip/chip/src/main/scala/TargetConfigs.scala
Lines 267 to 299 in e056889
| // Gemmini NN accel config, base off chipyard's GemminiRocketConfig | |
| //****************************************************************** | |
| class FireSimGemminiRocketConfig extends Config( | |
| new WithDefaultFireSimBridges ++ | |
| new WithFireSimConfigTweaks ++ | |
| new chipyard.GemminiRocketConfig) | |
| class FireSimLeanGemminiRocketConfig extends Config( | |
| new WithDefaultFireSimBridges ++ | |
| new WithFireSimConfigTweaks ++ | |
| new chipyard.LeanGemminiRocketConfig) | |
| class FireSimLeanGemminiPrintfRocketConfig extends Config( | |
| new WithDefaultFireSimBridges ++ | |
| new WithFireSimConfigTweaks ++ | |
| new chipyard.LeanGemminiPrintfRocketConfig) | |
| //********************************************************************************** | |
| // Supernode Configurations, base off chipyard's RocketConfig | |
| //********************************************************************************** | |
| class SupernodeFireSimRocketConfig extends Config( | |
| new WithFireSimHarnessClockBridgeInstantiator ++ | |
| new chipyard.harness.WithHomogeneousMultiChip(n=4, new Config( | |
| new freechips.rocketchip.subsystem.WithExtMemSize((1 << 30) * 8L) ++ // 8GB DRAM per node | |
| new FireSimRocketConfig))) | |
| //********************************************************************************** | |
| //* CVA6 Configurations | |
| //*********************************************************************************/ | |
| class FireSimCVA6Config extends Config( | |
| new WithDefaultFireSimBridges ++ | |
| new WithFireSimConfigTweaks ++ | |
| new chipyard.CVA6Config) |
[P1] Guard FireSim configs behind optional generator discovery
Chipyard now removes Gemmini and CVA6 mixins from the core repo and only adds their projects when the submodule is initialized, but FireSim still unconditionally declares FireSimGemminiRocketConfig and FireSimCVA6Config in this file. When someone follows the new minimal submodule setup (leaving generators/gemmini or generators/cva6 uninitialized), sbt firechip/compile fails with not found: type chipyard.GemminiRocketConfig/chipyard.CVA6Config because the classes live in those optional repos and are no longer on the classpath. These configs should either live alongside their generator or be conditionally generated only when the corresponding submodule is present so that FireSim can compile without optional accelerators.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
|
Oh amazing. Ty |
Related PRs / Issues:
Type of change:
Impact:
Contributor Checklist:
mainas the base branch?changelog:<topic>label?changelog:label?.conda-lock.ymlfile if you updated the conda requirements file?Please Backport?CI Help:
Add the following labels to modify the CI for a set of features.
Generally, a label added only affect subsequent changes to the PR (i.e. new commits, force pushing, closing/reopening).
See
ci:*for full list of labels:ci:fpga-deploy- Run FPGA-based E2E testingci:local-fpga-buildbitstream-deploy- Build local FPGA bitstreams for platforms that are releasedci:disable- Disable CI