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

[WIP] Synlig, new system-verilog tool integration #2841

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

Conversation

amirarjmand93
Copy link
Contributor

@amirarjmand93 amirarjmand93 commented Dec 8, 2024

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

  • Bug fix (change which fixes an issue)
  • New feature (change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

@github-actions github-actions bot added infra Project Infrastructure lang-python Python code lang-make CMake/Make code external_libs labels Dec 8, 2024
@github-actions github-actions bot added the lang-shell Shell scripts (bash etc.) label Dec 8, 2024
@amirarjmand93 amirarjmand93 changed the title synlig, new system verilog tool integration synlig, new system-verilog tool integration Dec 8, 2024
@amirarjmand93
Copy link
Contributor Author

@vaughnbetz This branch is working on the local pc fine without any problem. but in the CI test, I am getting some errors related to JAVA dependency while making files. for Synlig we need some packages to be installed:

apt install -y gcc-11 g++-11 build-essential cmake tclsh ant default-jre swig google-perftools libgoogle-perftools-dev python3 python3-dev python3-pip uuid uuid-dev tcl-dev flex libfl-dev git pkg-config libreadline-dev bison libffi-dev wget python3-orderedmultidict

I tried to append some packages from this list that are missing from our flow. (like default-jre). I added them to the .github folder inside three bash files(in the figure) but I think I should add them in other files also.

image

This error arises from most of the ci tests:
image

@amirarjmand93 amirarjmand93 changed the title synlig, new system-verilog tool integration Synlig, new system-verilog tool integration Dec 8, 2024
@vaughnbetz
Copy link
Contributor

@AlexandreSinger @soheilshahrouz : any ideas?

@github-actions github-actions bot added the scripts Utility & Infrastructure scripts label Dec 9, 2024
@amirarjmand93 amirarjmand93 changed the title Synlig, new system-verilog tool integration [WIP] Synlig, new system-verilog tool integration Dec 10, 2024
@amirarjmand93
Copy link
Contributor Author

why hasn't the nightly test been launched?

@vaughnbetz
Copy link
Contributor

We had to turn off automatic launch of the nightly tests as we don't have the google cloud going. You should run some or all of the nightly tests manually if this code change could break them. @AlexandreSinger : not sure if you have a button that can launch the nightly tests on one of our SAVI servers or not yet.

@github-actions github-actions bot added the lang-hdl Hardware Description Language (Verilog/VHDL) label Dec 16, 2024
@amirarjmand93
Copy link
Contributor Author

amirarjmand93 commented Dec 16, 2024

1- vtr_reg_system_verilog.log.txt
2- parmys_reg_strong.log.txt

The two tests from the nightly tests related to SystemVerilog were run successfully with Synlig enabled(cmake: -DYOSYS_F4PGA_PLUGINS=ON) during the make process. Other nightly tests generate VTR while Synlig (formerly known as cmake: -DYOSYS_F4PGA_PLUGINS) is disabled(OFF), and there should be no issues with the new integration.

image

Copy link
Contributor

@vaughnbetz vaughnbetz left a comment

Choose a reason for hiding this comment

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

Thanks for handling this! I have some suggested changes to reduce code duplication and clarify some messages/comments.

@@ -0,0 +1,65 @@
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like you have 3 copies of the make_sv_flattened.py script. Why not put it in the base f4pga directory instead and just call it from there (with ../../make_sv_flattened.py)? Or put it in the search path.


# Separate opt for Parmys execution(verilog or system-verilog)
if {$env(PARSER) == "default"} {
puts "Running Parmys with disables additional passes "
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this output means. Do you mean to say "Running parmys with no additional passes." ?

parmys -a QQQ -nopass -c CCC YYY

} elseif {$env(PARSER) == "system-verilog" || $env(PARSER) == "surelog"} {
puts "Running Parmys with Additional Passes Resolve Conflicts"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this output means. Wouldn't it be more clear to say "Running Parmys with additional passes to flatten all files and parse SystemVerilog" ?

@@ -65,9 +65,9 @@ jobs:
- {test: "vtr_reg_strong", cores: "16", options: "", cmake: "-DVTR_ASSERT_LEVEL=3", extra_pkgs: "libeigen3-dev"}
- {test: "vtr_reg_strong_odin", cores: "16", options: "", cmake: "-DVTR_ASSERT_LEVEL=3 -DWITH_ODIN=ON", extra_pkgs: "libeigen3-dev"}
- {test: "vtr_reg_strong_odin", cores: "16", options: "-skip_qor", cmake: "-DVTR_ASSERT_LEVEL=3 -DVTR_ENABLE_SANITIZE=ON -DWITH_ODIN=ON", extra_pkgs: "libeigen3-dev"}
# - {test: "vtr_reg_system_verilog", cores: "16", options: "", cmake: "-DYOSYS_F4PGA_PLUGINS=ON", extra_pkgs: ""} # Test turned off -> F4PGA conflicts with Yosys (version 42)
- {test: "vtr_reg_system_verilog", cores: "16", options: "", cmake: "-DYOSYS_F4PGA_PLUGINS=ON", extra_pkgs: ""} # Test turned off -> F4PGA conflicts with Yosys (version 42)
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment looks out of date -- it says this test is turned off, but you are turning it back on. Delete the comment.
Also is synlig part of f4pga_plugins? If not, it would be better to change the name of the cmake variable to synlig for clarity.

@amirarjmand93
Copy link
Contributor Author

I am aware of these comments and duplicates. I just want to ensure that we are technically finished with this update and the last commits. I will make the minor changes and let you know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external_libs infra Project Infrastructure lang-hdl Hardware Description Language (Verilog/VHDL) lang-make CMake/Make code lang-python Python code lang-shell Shell scripts (bash etc.) scripts Utility & Infrastructure scripts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants