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

Manager support for custom TARGET_PROJECT #1495

Merged
merged 4 commits into from
Apr 30, 2023
Merged

Conversation

sagark
Copy link
Member

@sagark sagark commented Apr 27, 2023

This PR updates the manager to support arbitrary TARGET_PROJECTs instead of only firesim. In effect, all mentions of triplets (consisting of (DESIGN, TARGET_CONFIG, PLATFORM_CONFIG)) become quadruplets (consisting of (TARGET_PROJECT, DESIGN, TARGET_CONFIG, PLATFORM_CONFIG)).

This adds support for all manager features except producing the command to run a simulation (this needs some more thinking). But building bitstreams/metasims and doing everything up to and including infrasetup (and then running the sim by hand on the run farm instance) works as expected.

This should be fully backwards compatible with existing AGFIs and config yaml files (see below).

This also adds a fix so that the manager doesn't complain about deploy_quadruplet_override (formerly deploy_triplet_override) being set for vitis bitstreams. FYI: @abejgonzalez

Internally, you will notice that some mentions of "triplet" remain. These are because the make-system still relies on triplets for naming several things (e.g. directories in sim/generated-src/f1/). Updating this is punted to a future PR since this is only really a problem if you work with multiple different quadruplets that have the same triplet, which is rare. This has no effect on functionality besides this case.

Related PRs / Issues

N/A

UI / API Impact

  • Entries in config_hwdb.yaml now have a deploy_quadruplet_override key instead of deploy_triplet_override.
  • Entries in config_build_recipes.yaml now have a deploy_quadruplet key instead of deploy_triplet.

For both, backwards compatibility is ensured like so:

  1. The manager will print a warning if only deploy_triplet_override or deploy_triplet are defined, instructing the user to upgrade, but will otherwise work as usual.
  2. The manager will error if both deploy_triplet_override and deploy_quadruplet_override are defined or both deploy_triplet and deploy_quadruplet are defined.
  3. The manager will accept "triplet" values supplied for any of these keys and automatically assume TARGET_PROJECT=firesim for any triplet. Otherwise, quadruplets are expected and all defaults in config files and docs have been updated to quadruplets.

The serialized versions of triplets stored in AGFI descriptions (firesim-buildtriplet and firesim-deploytriplet) continue to be kept so that old managers can continue to use AGFIs generated with the version of the manager in this PR. For AGFIs tagged only with a triplet, the manager assumes TARGET_PROJECT=firesim. This version of the manager also adds firesim-buildquadruplet and firesim-deployquadruplet key/values on AGFIs for the new TARGET_PROJECT support. Old managers will ignore these keys silently.

I have tested:

  • Existing AGFIs booting Linux
  • Non-firesim TARGET_PROJECT AGFI build
  • Non-firesim TARGET_PROJECT FPGA-run (infrasetup run by hand on run farm)
  • Non-firesim TARGET_PROJECT metasims (infrasetup run by hand on run farm)

Verilog / AGFI Compatibility

See note above. No user facing changes. No need for AGFI re-gen.

Contributor Checklist

  • Is this PR's title suitable for inclusion in the changelog and have you added a changelog:<topic> label?
  • Did you add Scaladoc/docstring/doxygen to every public function/method?
  • Did you delete any extraneous prints/debugging code?
  • Did you state the UI / API impact?
  • Did you specify the Verilog / AGFI compatibility impact?
  • If applicable, did you apply the ci:fpga-deploy label?

Reviewer Checklist (only modified by reviewer)

Note: to run CI on PRs from forks, comment @Mergifyio copy main and manage the change from the new PR.

  • Is the title suitable for inclusion in the changelog and does the PR have a changelog:<topic> label?
  • Did you mark the proper release milestone?
  • Did you check whether all relevant Contributor checkboxes have been checked?

@sagark sagark added changelog:added Put PR title in 'Added' section of changelog ci:fpga-deploy labels Apr 27, 2023
Copy link
Contributor

@abejgonzalez abejgonzalez left a comment

Choose a reason for hiding this comment

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

SFTM Overall

Comment on lines 285 to 288
assert len(tag_build_quadruplet) <= 255, "ERR: aws does not support tags longer than 256 chars for build_quadruplet"
assert len(tag_deploy_quadruplet) <= 255, "ERR: aws does not support tags longer than 256 chars for deploy_quadruplet"
assert len(tag_build_triplet) <= 255, "ERR: aws does not support tags longer than 256 chars for build_triplet"
assert len(tag_deploy_triplet) <= 255, "ERR: aws does not support tags longer than 256 chars for deploy_triplet"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this 255? Shouldn't this be 2048 (each tag length can be up to 2048)?

@sagark sagark enabled auto-merge April 28, 2023 22:48
@sagark sagark disabled auto-merge April 30, 2023 19:45
@sagark sagark merged commit be0ef0d into main Apr 30, 2023
@sagark sagark deleted the manager-custom-targets branch April 30, 2023 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:added Put PR title in 'Added' section of changelog ci:fpga-deploy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants