Skip to content
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

Have FireSim build recipes use Chipyard configs rather than FireChip configs #695

Merged
merged 8 commits into from
Oct 16, 2020

Conversation

alonamid
Copy link
Contributor

@alonamid alonamid commented Oct 15, 2020

Related issue:

Type of change: new feature

Impact: other

Release Notes

Have FireSim build recipes use shared Chipyard configs rather than custom FireChip configs. This required updating the Chipyard phase to enable config strings to accept config fragments from several different packages.

Goes together with FireSim PR 645 firesim/firesim#645

@jerryz123
Copy link
Contributor

Docs pls

@alonamid
Copy link
Contributor Author

touché. Added docs update

Copy link
Contributor

@davidbiancolin davidbiancolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we just remove the special treatment of "WithDefaultMemModel" if we're going to add it to the config tweaks. It'd simplify a bunch of the concrete configs.

docs/Simulation/FPGA-Accelerated-Simulation.rst Outdated Show resolved Hide resolved
docs/Simulation/FPGA-Accelerated-Simulation.rst Outdated Show resolved Hide resolved
docs/Simulation/FPGA-Accelerated-Simulation.rst Outdated Show resolved Hide resolved
The simplest method to add this config fragments to your custom Chipyard config is through FireSim's build recipe scheme.
After your FireSim environment is setup, you will define your custom build recipe in ``sims/firesim/deploy/deploy/config_build_recipes.ini``. By prepending the FireSim config fragments (separated by ``_``) to your Chipyard configuration, these config fragments will be added to your custom configuration as if they were listed in a custom Chisel config class definition. For example, if you would like to convert the Chipyard ``LargeBoomConfig`` to a FireSim simulation with a DDR3 memory model, the appropriate FireSim ``TARGET_CONFIG`` would be ``DDR3FRFCFSLLC4MB_WithDefaultFireSimBridges_WithFireSimConfigTweaks_chipyard.LargeBoomConfig``. Note that the FireSim config fragments are part of the ``firesim.firesim`` scala package, and therefore there do not need to be prefixed with the full class name, and opposed to the Chipyard config fragments which need to be prefixed with the chipyard package name.

An alternative method to prepending the FireSim config fragments in the FireSim build recipe is to create a new "permanent" FireChip custom configuration, which includes the FireSim config fragments.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually alright to put the class not in FireChip. They'd just have to change the default package to point at their own and that package would have to depend on the firechip package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in this case its better to specify the path of least resistance, which is adding to FireChip. I would try to avoid leading people down the build system dependancy chain.

docs/Simulation/FPGA-Accelerated-Simulation.rst Outdated Show resolved Hide resolved
@jerryz123
Copy link
Contributor

We don't need most of the FireSimXConfig s defined in Targetconfigs.scala anymore, right?

@alonamid
Copy link
Contributor Author

I was debating whether to remove those or not.
The ones that could be removed are:
FireSimLargeBoomConfig
FireSimLargeBoomAndRocketConfig
FireSimGemminiRocketConfig
FireSimQuadRocketConfig
FireSimArianeConfig

Copy link
Contributor

@davidbiancolin davidbiancolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm not sure how i personally feel about removing the configs entirely. We might want to deprecate them, and keep them around for one more release cycle. In any case, that will be left to a later PR.

@alonamid alonamid merged commit 46ea900 into dev Oct 16, 2020
@alonamid alonamid mentioned this pull request Nov 24, 2020
@jerryz123 jerryz123 deleted the shared-configs branch October 1, 2022 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants