Skip to content

Add Xilinx arch tests to CI #3120

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

Merged
merged 8 commits into from
Jun 14, 2025

Conversation

WhiteNinjaZ
Copy link
Contributor

Description

This PR adds Xilinx tests to CI.

Related Issue

Part of the fix for #3117

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)

@AmirhosseinPoolad
Copy link
Contributor

AmirhosseinPoolad commented Jun 7, 2025

Thanks for the PR @WhiteNinjaZ. The test failures appear to be because the results are "too good." I am a little bit suspicious about the ~40% decrease in the number of pre-packed blocks. @AlexandreSinger Did we upgrade our synthesis tools since October last year?

Regardless, if everything is fine make sure you also update the golden results in regression_tests/vtr_reg_nightly_test2/vtr_xilinx_qor in addition to updating the strong tests golden results.

@vaughnbetz
Copy link
Contributor

Looks like the circuits shrank, including a 10% reduction in IOs and a larger (35% or so) reduction in logic. Maybe better sweeping of unused logic?
Worth looking at the larger designs to see if they also show a big change. @WhiteNinjaZ : if you can look at the logs and decide if this is OK or not that would be good.

@WhiteNinjaZ
Copy link
Contributor Author

WhiteNinjaZ commented Jun 12, 2025

@AmirhosseinPoolad and @vaughnbetz The ~40% decrease in pre-packed blocks comes from the strong tests which are pretty outdated. Unfortunately, I don't totally remember exactly what happened between now and the last time the golden was updated for the strong tests but here is what I know:

The strong tests work on a very dumbed down version of the xilinx arch. So its not the most accurate. From what I can tell, the last time the QOR for the strong tests where updated was in 2022. So, there have been significant changes to the code base since the last time the QOR was updated. It might be worth removing the simple-7series.xml checks altogether since that was mostly a placeholder check for making sure that the diagonal wiring connections didn't break while we worked on refining the actual architecture. I could have the strong tests use the flagship architecture instead. That does introduce more complexity though since the flagship has things like carry chains, multipliers, and BRAMs that are not present in the simple arch.

Unfortunately, I can't really compare QOR on the latest branch of VTR for the flagship Xilinx arch run in nightly (7series_BRAM_DSP_carry.xml) because of issue #3117 but looking at a rolled back version of VTR from about a month ago (May 1st) that I have locally, my QOR does end up being within spec when I run it again.

@WhiteNinjaZ
Copy link
Contributor Author

WhiteNinjaZ commented Jun 12, 2025

I think I will keep the simple-7series.xml arch in vtr_strong but I will add a new test for the flagship arch as well within the same test suite using one of the small designs.

@vaughnb-cerebras
Copy link

Sounds good. I am neutral on the simple arch, but we should definitely have at least one (fast) test on the full 7-series arch in the strong tests so it gets run by CI every time.

Copy link
Contributor

@AlexandreSinger AlexandreSinger left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @WhiteNinjaZ for looking into this! I am happy that you found the issue!

@AlexandreSinger AlexandreSinger merged commit 0c5c8ad into verilog-to-routing:master Jun 14, 2025
33 checks passed
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.

5 participants