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

Get VTR working on macOS again #2854

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

Conversation

Liamolucko
Copy link

VTR's support for macOS has bitrotted over time; this gets it working again.

Description

The only problems that were actually related to macOS were the missing includes and Parmys using .dylib instead of .so; the rest were just a result of the tests being run in a different order and can be reproduced on Linux with test_vpr --order rand.

vtr_reg_strong still has two failing tests: fix_clusters_test_arch/apex2 and k6_frac_N10_frac_chain_mem32K_40nm/or1200. or1200 fails on Linux too for me, so I didn't bother investigating further (it's saying that the circuit is unroutable).

After some cursory investigation, it seems like apex2 might be failing because two blocks which the .place file specifies should be in different locations are getting packed into the same cluster before the .place file is read. I don't know enough about how VPR works to figure out what's preventing that from happening on Linux as well (or anything about it at all really, and I could have completely misunderstood the issue).

Related Issue

Motivation and Context

How Has This Been Tested?

I ran the ctest unit tests, vtr_reg_basic, and vtr_reg_strong (which had the failures mentioned above. On Linux, k6_frac_N10_40nm/ex1010.pre-vpr also failed with an error saying it was unroutable, but only when using GCC; it passed when using Clang).

ctest and vtr_reg_basic were run via. this Nix package, and vtr_reg_strong was run in a nix develop shell for the same package.

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

The tests run in a different order on macOS than Linux, which happens to
result in this segfault.

The problem is that `test_compressed_grid` leaves some stuff behind in
`mutable_device().logical_block_types`, which have `pb_type` set to
`nullptr`. This then breaks `test_connection_router`'s call to
`XmlReadArch`, since it assumes that all of the logical block types are
ones that it created, all of which either have `pb_type` set or are
empty.

Making `test_compressed_grid` clear `logical_block_types` when it's done
fixes that.
The problem was again that tests weren't cleaning up after themselves
properly, and were then breaking subsequent tests: this time, they were
correctly calling `NocStorage::clear_noc`, but that was failing to clear
`router_id_conversion_table`.

Since `unordered_map::emplace` is used to add entries to that table,
writing to an entry that's already been filled doesn't do anything, and
so subsequent reads wil retrieve the data from previous tests: the
retrieved IDs are then used as indices, causing out-of-bounds array
accesses and an eventual crash.
Yosys expects its plugins to be .so files, even on macOS. The Make
backend already did this correctly, but CMake was still using the
default of `.dylib` for macOS.

This gets vtr_reg_basic working on macOS.
@github-actions github-actions bot added VPR VPR FPGA Placement & Routing Tool libarchfpga Library for handling FPGA Architecture descriptions lang-cpp C/C++ code lang-make CMake/Make code Parmys labels Jan 5, 2025
@hzeller
Copy link
Contributor

hzeller commented Jan 6, 2025

(off topic: I think it would be good if you contributed the nix package to https://github.com/NixOS/nixpkgs ... just this morning I was looking if vtr needs to be verion-updated in nixpkgs ... and realized that it was actually not even there. So having it in the regular nixpkgs would actually be very nice)

@vaughnbetz
Copy link
Contributor

Thanks! The changes look OK; launching CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang-cpp C/C++ code lang-make CMake/Make code libarchfpga Library for handling FPGA Architecture descriptions Parmys VPR VPR FPGA Placement & Routing Tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants