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

Timing annotation #362

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Timing annotation #362

wants to merge 15 commits into from

Conversation

apond308
Copy link
Contributor

Motivate of the pull request

  • To address an existing issue. If so, please provide a link to the issue:
  • Breaking new feature. If so, please describe details in the description part.

Describe the technical details

What is currently done? (Provide issue link if applicable)

Currently, the architecture timing values are hardcoded into the arch files.

What does this pull request change?

It adds timing annotation files so that the timing values are read into variables in the arch files.

Which part of the code base require a change

  • VPR
  • Tileable routing architecture generator
  • OpenFPGA libraries
  • FPGA-Verilog
  • FPGA-Bitstream
  • FPGA-SDC
  • FPGA-SPICE
  • Flow scripts
  • Architecture library
  • Cell library
  • Documentation
  • Regression tests
  • Continous Integration (CI) scripts

Impact of the pull request

  • Require a change on Quality of Results (QoR)
  • Break back-compatibility. If so, please list who may be influenced.

@apond308 apond308 marked this pull request as ready for review July 23, 2021 18:08
Copy link
Collaborator

@tangxifan tangxifan left a comment

Choose a reason for hiding this comment

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

@apond308 Some comments on the details. This PR is on the right track. Just need some effort to make it clean.

Since you have added a few benchmarks and new architectures, can you add new test cases?

@@ -0,0 +1,75 @@
# Run VPR for the 'and' design
Copy link
Collaborator

Choose a reason for hiding this comment

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

Depending on the test, you want to run, you may not need a new script.
If you just want to test bitstream generation, you can reuse this script: https://github.com/lnis-uofu/OpenFPGA/blob/master/openfpga_flow/openfpga_shell_scripts/generate_bitstream_global_tile_multiclock_example_script.openfpga

On the other side, the VexRiscV and picorv benchmarks are too big to be tested on CI (iVerilog may takes hours to finish). I suggest just testing bitstream on CI.

Also to avoid adding new script, you may also consider to add options to the script which can be customized in task configuration file.
For example,

vpr ${VPR_ARCH_FILE} ${VPR_TESTBENCH_BLIF} --clock_modeling route ${OPENFPGA_VPR_DEVICE_LAYOUT}

You can define the variable ${OPENFPGA_<your_own_variable_name>}

In task configuration file, you can define the content of the variable:

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 see what you're saying about avoiding adding a new script, but how would you disable the '--clock_modeling route' option of vpr with a variable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes. Refer to the use of ${OPENFPGA_VPR_DEVICE_LAYOUT}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, also I needed to remove the '--activity_file ${ACTIVITY_FILE}' part of one of the 'link_openfpga_arch' line as well. Since that is not determined by the config, what would be the best way to resolve that besides creating a new script?

openfpga_flow/tasks/skywater_openfpga_task Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants