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

Rationalize and refactor build system (part 2) #1422

Merged
merged 7 commits into from
Jan 21, 2025

Conversation

whitequark
Copy link
Member

@whitequark whitequark commented Jan 15, 2025

Depends on #1421.

Also allows splitting nextpnr-himbaechel per microarchitecture.

@whitequark whitequark requested a review from gatecat January 15, 2025 16:26
@whitequark whitequark marked this pull request as draft January 15, 2025 16:27
@whitequark whitequark force-pushed the catherine/refactor-cmake-part2 branch 4 times, most recently from 6303fbe to 0fc4fb2 Compare January 16, 2025 10:40
cmake/FindIceStorm.cmake Outdated Show resolved Hide resolved
@whitequark whitequark force-pushed the catherine/refactor-cmake-part2 branch 6 times, most recently from 0085bdd to a8e6330 Compare January 16, 2025 21:19
@whitequark whitequark changed the title [WIP] Rationalize and refactor build system (part 2) Rationalize and refactor build system (part 2) Jan 16, 2025
@whitequark whitequark force-pushed the catherine/refactor-cmake-part2 branch from a8e6330 to cd4f8a0 Compare January 16, 2025 22:01
This fully preserves existing functionality, although the `embed` mode
is untested and seems broken.
While it served a purpose (granting the ability to build `.bba` files
separately from the rest of nextpnr), it made things excessively
convoluted, especially around paths.

This commit removes the ability to pre-generate chip databases. As far
as I know, I was the primary user of that feature. It can be added back
if there is demand for it.

In exchange the per-family `CMakeLists.txt` files are now much easier
to understand.
@whitequark whitequark force-pushed the catherine/refactor-cmake-part2 branch from 3a4c296 to 9caffca Compare January 21, 2025 14:42
@whitequark whitequark marked this pull request as ready for review January 21, 2025 14:44
@whitequark whitequark force-pushed the catherine/refactor-cmake-part2 branch from 9caffca to 73509b9 Compare January 21, 2025 15:00
README.md Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
himbaechel/CMakeLists.txt Outdated Show resolved Hide resolved
Primarily, this commit makes both of them use the `BBAsm` functions
to build and compile `.bba` files.

In addition, Himbaechel targets are now aligned with the rest in
how they are configured: instead of having all uarches enabled with
all of the devices disabled (the opposite of the rest of nextpnr),
uarches must be enabled explicitly but they come with all devices
enabled (except for Xilinx, which does not have a list of devices).
This removes the atomic rename for bbasm outputs because it embeds
the resulting paths into the `.cc` files in embed mode. In any case
the write should be fast enough to not be a big risk for interrupted
builds.

This was tested with Clang 19 only (gcc hasn't had a release that
supports `#embed` yet).
Two user-visible changes were made:
* `-DUSE_RUST` is replaced with `-DBUILD_RUST`, by analogy with
  `-DBUILD_PYTHON`
* `-DCOVERAGE` was removed as it doesn't work with either modern GCC
  or Clang
The impetus for this commit is the fact that it causes rare but
build-breaking race conditions when used with `make -jN` with `N > 1`.
These race conditions are difficult to track down or fix because of
the very rudmentary debugging tools provided by `make` and opaque
semantics of CMake's Makefile generator. They break the build by
running two `.bba` generation processes, then one of them renaming
the `.bba.new` file once it's done, leaving the other one to fail.

After reflection (as the author of this code path) and discussion with
community members who use it, I've concluded that this isn't the right
approach.
1. In practice, on targets where `-DSERIALIZE_CHIPDBS=` matters, you
   also care about other build steps, like linking nextpnr, which
   are not serializable this way. So you use a workaround anyway, like
   `make`ing individual targets instead.
2. The way to serialize the build with Make is the `-j1` option. Trying
   to work around `-jN` to make it work like `-j1` is inherently error
   prone. While there is some utility in not serializing C++ compilation
   this utility could be more easily achieved by providing a single
   target that builds all chipdbs, running `make <chipdb-target> -j1`,
   then running `make -jN` for the rest of the build.
@whitequark whitequark force-pushed the catherine/refactor-cmake-part2 branch from 73509b9 to fdc0198 Compare January 21, 2025 16:55
@whitequark whitequark merged commit 17943a5 into master Jan 21, 2025
16 checks passed
@whitequark whitequark deleted the catherine/refactor-cmake-part2 branch January 21, 2025 17:13
@mmicko
Copy link
Member

mmicko commented Jan 22, 2025

@whitequark there are couple of things that are now broken (when building oss-cad-suite but some are general)

minor issues (when doing in-source-tree build):

  • .bba and .cc as product are now generated in same directory as others so not covered by .gitignore, previously it was in chipdb subdirectory
  • since "generated" subdirectory is not used any more .gitignore needs updating to cover those files

major issues :

  • there is no target just to generate BBA files and nothing else for architecture
  • there is no way to tell build system to use already generated BBA files from other location

Since generating BBA files takes time but always generate same output (for same endianess) and sometimes specific binaries/scripts only for this, this was used on oss-cad-suite build to minimize build time and lower redudancy.

Using nextpnr-arch-chipdb target we can get BBA created (even at the wrong location right now) but it also lose some time on generating .cc files, splitting these two into two targets and making a way to propagate BBA_CHIPDB location for bba files instead would help fixing this.

@whitequark
Copy link
Member Author

whitequark commented Jan 22, 2025

when doing in-source-tree build

I would rather disallow in-source-tree builds entirely. They cause many issues and with CMake in particular there are just way too many auxiliary files to keep track of. Projects like LLVM already do that for similar reasons.

@gatecat What do you think?

  • there is no target just to generate BBA files and nothing else for architecture
  • there is no way to tell build system to use already generated BBA files from other location

The change that resulted in this was done quite intentionally since the existing system for generating BBA files was unmaintainable itself and also an unmanageable obstacle to changing how the build system works.

Pre-running bbasm is something I have no plans of allowing again. If you are bottlenecked on .cc file generation/compilation then you should use a compiler with #embed directive support (Clang 19 or later) or use -DEXTERNAL_CHIPDB=ON (which aligns legacy architectures with Himbaechel architectures). There might be issues in how -DEXTERNAL_CHIPDB=ON interacts with relative .../share path discovery but we could just copy that code from Himbächel to legacy architecture's chipdb discovery code.

Pre-running .bba file generation is something that I decided could be restored later as a capability if it becomes an issue while working on this refactoring. However, I want to say that I am also packaging the entirety of nextpnr for YoWASP, and my builds always finish within about 30 minutes, despite using one job and not several as oss-cad-suite does. Is it really a major issue if it doesn't take that much time?

@whitequark
Copy link
Member Author

I'm going to add export/import of generated .bba files since it was more widely used/needed than I anticipated. It shouldn't take too long.

@whitequark
Copy link
Member Author

I've implemented this functionality in #1436, please let me know if it works. The nextpnr-all-bba target (or nextpnr-<arch>-bba) doesn't depend on anything but the .bba files themselves and the EXPORT_BBA_FILES directory doesn't contain anything extra. The .cc files are generated as a part of the subsequent build with IMPORT_BBA_FILES so I think it should solve the issue of that taking a long time also.

I don't know how useful it is, but you can also do a normal build with -DEXPORT_BBA_FILES enabled and it will both build nextpnr and export .bba files.

@cr1901 You should be able to run make nextpnr-all-bba -j1 target as a part of a normal build as well, to serialize producing these files.

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.

3 participants