[ImportVerilog] Bump `slang`

[hovind]

trafficstars

Some groundwork towards using a more up to date slang verison.

TODO

• [ ] Proper error message on unsupported compilers.
• [ ] Don't match on OS specifics: https://github.com/llvm/circt/pull/7792#discussion_r1835856699.
• [x] ~Figure out why https://github.com/llvm/circt/pull/7792/commits/f4d4ae91cfac5be8bea3eb95e0563de57ae59405 is needed.~ Resolved in https://github.com/llvm/circt/pull/7847/commits/aca1839f02ed75481f08af98c0e0dad054ece158.
• [ ] Please separate the build infrastructure changes from the rest of the PR. https://github.com/llvm/circt/pull/7792#discussion_r1935982014
• [x] If boost is escaping into the API, we should probably be picking up the boost from the slang build, or at least guaranteeing it's the same version without hardcoding it here. https://github.com/llvm/circt/pull/7792#discussion_r1884669955

[hovind]

Currently fails in CI due to

GCC

2024-11-10T17:59:53.5774363Z FAILED: _deps/slang-build/source/CMakeFiles/slang_slang.dir/diagnostics/DiagnosticClient.cpp.o 
2024-11-10T17:59:53.5790822Z /usr/bin/g++ -DSLANG_BOOST_SINGLE_HEADER -DSLANG_STATIC_DEFINE -D_DEBUG -D_GLIBCXX_ASSERTIONS -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/__w/circt/circt/llvm/install/include -I/__w/circt/circt/include -I/__w/circt/circt/build/include -I/__w/circt/circt/build/_deps/slang-src/source/../include -I/__w/circt/circt/build/_deps/slang-build/source -isystem /__w/circt/circt/build/_deps/slang-src/source/../external -isystem /__w/circt/circt/build/_deps/fmt-src/include -O3 -DNDEBUG -fvisibility=hidden -fvisibility-inlines-hidden -UNDEBUG -std=c++2a -MD -MT _deps/slang-build/source/CMakeFiles/slang_slang.dir/diagnostics/DiagnosticClient.cpp.o -MF _deps/slang-build/source/CMakeFiles/slang_slang.dir/diagnostics/DiagnosticClient.cpp.o.d -o _deps/slang-build/source/CMakeFiles/slang_slang.dir/diagnostics/DiagnosticClient.cpp.o -c /__w/circt/circt/build/_deps/slang-src/source/diagnostics/DiagnosticClient.cpp
2024-11-10T17:59:53.5806008Z In file included from /__w/circt/circt/build/_deps/slang-src/source/../include/slang/diagnostics/DiagnosticEngine.h:16,
2024-11-10T17:59:53.5807967Z                  from /__w/circt/circt/build/_deps/slang-src/source/../include/slang/diagnostics/DiagnosticClient.h:10,
2024-11-10T17:59:53.5809625Z                  from /__w/circt/circt/build/_deps/slang-src/source/diagnostics/DiagnosticClient.cpp:8:
2024-11-10T17:59:53.5811625Z /__w/circt/circt/build/_deps/slang-src/source/../include/slang/diagnostics/Diagnostics.h:11:10: fatal error: concepts: No such file or directory
2024-11-10T17:59:53.5813055Z    11 | #include <concepts>

concepts was implemented in gcc-10: https://gcc.gnu.org/gcc-10/changes.html#cxx, while CI has

$ docker run ghcr.io/circt/images/circt-ci-build:20240213211952 /usr/bin/gcc --version
gcc (Ubuntu 9.4.0-1ubuntu1~20.04.2) 9.4.0

as far as I can tell.

clang

2024-11-10T18:01:25.2568466Z CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/lib/ccache/clang++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/__w/circt/circt/build/tools/circt/lib/Conversion/ImportVerilog -I/__w/circt/circt/lib/Conversion/ImportVerilog -I/__w/circt/circt/build/include -I/__w/circt/circt/llvm/llvm/include -I/__w/circt/circt/include -I/__w/circt/circt/build/tools/circt/include -I/__w/circt/circt/build/_deps/slang-src/source/../include -I/__w/circt/circt/build/_deps/slang-build/source -I/__w/circt/circt/build/_deps/slang-src/source/../external -I/__w/circt/circt/build/_deps/fmt-src/include -isystem /__w/circt/circt/llvm/llvm/../mlir/include -isystem /__w/circt/circt/build/tools/mlir/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -fvisibility-inlines-hidden  -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -Wno-non-virtual-dtor -Wno-ctad-maybe-unsupported -Wno-covered-switch-default -std=c++20 -MD -MT tools/circt/lib/Conversion/ImportVerilog/CMakeFiles/obj.CIRCTImportVerilog.dir/Statements.cpp.o -MF tools/circt/lib/Conversion/ImportVerilog/CMakeFiles/obj.CIRCTImportVerilog.dir/Statements.cpp.o.d -o tools/circt/lib/Conversion/ImportVerilog/CMakeFiles/obj.CIRCTImportVerilog.dir/Statements.cpp.o -c /__w/circt/circt/lib/Conversion/ImportVerilog/Statements.cpp
2024-11-10T18:01:25.2599620Z In file included from /__w/circt/circt/lib/Conversion/ImportVerilog/Statements.cpp:9:
2024-11-10T18:01:25.2601788Z In file included from /__w/circt/circt/lib/Conversion/ImportVerilog/ImportVerilogInternals.h:19:
2024-11-10T18:01:25.2604162Z In file included from /__w/circt/circt/build/_deps/slang-src/source/../include/slang/ast/ASTVisitor.h:10:
2024-11-10T18:01:25.2606447Z In file included from /__w/circt/circt/build/_deps/slang-src/source/../include/slang/ast/Constraints.h:10:
2024-11-10T18:01:25.2608744Z In file included from /__w/circt/circt/build/_deps/slang-src/source/../include/slang/ast/Statements.h:10:
2024-11-10T18:01:25.2610961Z In file included from /__w/circt/circt/build/_deps/slang-src/source/../include/slang/ast/Expression.h:10:
2024-11-10T18:01:25.2613180Z In file included from /__w/circt/circt/build/_deps/slang-src/source/../include/slang/ast/ASTContext.h:12:
2024-11-10T18:01:25.2615377Z In file included from /__w/circt/circt/build/_deps/slang-src/source/../include/slang/ast/Lookup.h:11:
2024-11-10T18:01:25.2617701Z In file included from /__w/circt/circt/build/_deps/slang-src/source/../include/slang/diagnostics/Diagnostics.h:18:
2024-11-10T18:01:25.2620056Z In file included from /__w/circt/circt/build/_deps/slang-src/source/../include/slang/text/SourceLocation.h:14:
2024-11-10T18:01:25.2622749Z /__w/circt/circt/build/_deps/slang-src/source/../include/slang/util/Hash.h:19:14: fatal error: 'boost/unordered/unordered_flat_map.hpp' file not found

The clang seems to me to be missing -DSLANG_BOOST_SINGLE_HEADER, which is correctly set in the gcc build.

[darthscsi]

Can we structure things so that slang inclusion depends on the c++ version and EH/RTTI (if still relevant) rather than forcing circt to that version? This is a bit more aggressive a language versioning than llvm projects tend to push.

[hovind]

Can we structure things so that slang inclusion depends on the c++ version and EH/RTTI (if still relevant) rather than forcing circt to that version? This is a bit more aggressive a language versioning than llvm projects tend to push.

Sorry, I'm not sure I follow. This is hidden behind the CIRCT_SLANG_FRONTEND_ENABLED define, should you not get circt-verilog unless you have CIRCT_SLANG_FRONTEND_ENABLED=ON and your compiler supports C++20?

This is further complicated by the fact that it is not enough for a compiler to support C++20. This makes cmake's CMAKE_CXX_STANDARD_REQUIRED=ON quite useless.

E.g. clang-16 supports a lot of C++20 constructs (https://clang.llvm.org/cxx_status.html#cxx20), but attempting to compile with CIRCT_SLANG_FRONTEND_ENABLED=ON and clang-16 gives quite a few ambiguous operator errors due to https://timsong-cpp.github.io/cppwp/n4868/over.match#oper-3.4.4. These errors are fixed in clang-17.

[fabianschuiki]

Really cool that this passes all the CI checks! Regarding EH/RTTI: it would be great to enforce C++20 and EH/RTTI if the user sets CIRCT_SLANG_FRONTEND_ENABLED, instead of additionally checking for C++20 and EH/RTTI settings. As in: if the user says they want the Slang frontend, compilation should fail if any other prerequisite is not met. Otherwise you run into this weird behaviour where you say you want Slang but you still don't get it because something else may be missing.

[towoe]

Hi @hovind, thanks for working on this! I would be eager to see a more recent slang version in CIRCT. It looks like this is mostly done, do you have capacity to finish this PR? I haven't checked all requirements and changes on the Slang side, but they recently had a v8.0 release which might include a few of the changes needed here?

[hovind]

Hi @hovind, thanks for working on this! I would be eager to see a more recent slang version in CIRCT. It looks like this is mostly done, do you have capacity to finish this PR? I haven't checked all requirements and changes on the Slang side, but they recently had a v8.0 release which might include a few of the changes needed here?

Hi @towoe, thanks for taking an interest! I have done a bit of work on the branch https://github.com/llvm/circt/compare/main...hovind:circt:dev/hovind/slang-8, but ran into

Failed Tests (1):
  CIRCT :: Tools/circt-verilog-lsp-server/diagnostic.test

Due to a missing unused net warning.

The surface area for slang has increased a bit since I started this, with the added circt-verilog-lsp-server.

I haven't had time after work to look more into this, and don't think I will in the immediate future. If anyone wants to help push it over the finishing line, I would be happy to respond to any questions to the best of my ability.

[towoe]

Thanks for the quick response @hovind and also the continued efforts on the slang-8 branch! I try to find some time and see if I can support this.

[towoe]

I think I found the reason for the falling test. Added that and few other things, can be found here dev/towoe/slang-8-build

We still need to separate out the CI update. There is another open PR #7196 but this seem a bit stale (and not really changing anything anymore?). @darthscsi

Creating a PR against the feature branch of this PR is a bit confusing, any suggestions how to proceed?

[hovind]

I think I found the reason for the falling test. Added that and few other things, can be found here dev/towoe/slang-8-build

We still need to separate out the CI update. There is another open PR #7196 but this seem a bit stale (and not really changing anything anymore?). @darthscsi

Creating a PR against the feature branch of this PR is a bit confusing, any suggestions how to proceed?

Good find!

I don't think the renames from clang to clang-17 are strictly necessary, I think I did them to make it symmetric with gcc.

$ docker run --rm -it --entrypoint /bin/bash ghcr.io/circt/images/circt-ci-build:20241113160334  -c "gcc --version"
gcc (Ubuntu 9.4.0-1ubuntu1~20.04.2) 9.4.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ docker run --rm -it --entrypoint /bin/bash ghcr.io/circt/images/circt-ci-build:20241113160334  -c "clang --version"
Ubuntu clang version 17.0.6 (++20231208085846+6009708b4367-1~exp1~20231208085949.74)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

[hovind]

I think I found the reason for the falling test. Added that and few other things, can be found here dev/towoe/slang-8-build

We still need to separate out the CI update. There is another open PR #7196 but this seem a bit stale (and not really changing anything anymore?). @darthscsi

Creating a PR against the feature branch of this PR is a bit confusing, any suggestions how to proceed?

I cherry-picked your changes here, tests are passing! :tada: I could force push the contents of that branch onto this one, if you have rights to push commits to this branch? Alternatively you could open a new pull request and I'll close this one.

[towoe]

I cherry-picked your changes here, tests are passing! 🎉 I could force push the contents of that branch onto this one, if you have rights to push commits to this branch? Alternatively you could open a new pull request and I'll close this one.

I guess the best thing would be if you could update this PR with the latest changes. Thanks for still taking the time to help out!

[towoe]

The build fails because of a version check failing. Seems to me that in the short integration tests the git version is not picked up (the google benchmark below has version 0.0.0). Was able to reproduce it locally, but not yet sure where it comes from. Will look into it.

[fabianschuiki]

Really nice to see the tests pass :partying_face:

[fabianschuiki]

@towoe @hovind Should we rebase this onto main to see if it still passes CI, and then land the PR? It would be a shame to not land this after all the excellent work that went into this 😃. And if there are remaining things to tackle, we could do that in a follow-up PR. As long as CI passes, we're good.

[hovind]

@towoe @hovind Should we rebase this onto main to see if it still passes CI, and then land the PR? It would be a shame to not land this after all the excellent work that went into this 😃. And if there are remaining things to tackle, we could do that in a follow-up PR. As long as CI passes, we're good.

Thanks for helping to push this along @fabianschuiki!

Should probably separate out CI changes per @darthscsi's request in https://github.com/llvm/circt/pull/7792#discussion_r1884662217.

However, I don't think the compiler version changes are necessary if https://github.com/circt/images/pull/38 lands. Any idea if we are inclined to merge that one, @seldridge?

[seldridge]

@hovind: I merged that PR just now. Sorry about that. I didn't wind up actually needing that as I could point at the dated version of that. 🙃

[hovind]

@hovind: I merged that PR just now. Sorry about that. I didn't wind up actually needing that as I could point at the dated version of that. 🙃

No worries, thanks for the prompt merge! Could anyone fire off a https://github.com/circt/images/actions/workflows/circt-build.yml when they have a spare moment?

[seldridge]

Kicked off. (I'm not sure why that isn't setup on post-merge... 😓)

[fabianschuiki]

Ran through successfully: https://github.com/circt/images/actions/runs/15048277474

[hovind]

Thanks @seldridge and @fabianschuiki!

$ docker run --rm -it --entrypoint /usr/bin/gcc ghcr.io/circt/images/circt-integration-test:v18.2 --version
gcc (Ubuntu 9.4.0-1ubuntu1~20.04.2) 9.4.0

If I'm not mistaken, the integration test image only seems to be published when we mint a release on https://github.com/circt/images. We probably have to update that image as well?

The circt-ci-build image looks great now:

$ docker run --rm -it --entrypoint /usr/bin/gcc ghcr.io/circt/images/circt-ci-build:20250515145637 --version
gcc (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0

[fabianschuiki]

Ugh I keep forgetting that the images only publish upon a new release. Sorry about that @hovind. I've tagged v19 in the circt/images repository. Build is running now. It looks like this is the only image that depends on a tag. I'm wondering whether we should align this more with the other images which just push whenever they were successful 🤔 WDYT @seldridge @mikeurbach?

[mikeurbach]

I honestly didn't know circt/images had any other set up, I've always just followed the steps on the README. If we build automatically on push after successful CI, would it just get an automated version based on the date / commit? I don't think we gain much by having to make github releases, so that would be fine by me.

[hovind]

Ugh I keep forgetting that the images only publish upon a new release. Sorry about that @hovind. I've tagged v19 in the circt/images repository. Build is running now. It looks like this is the only image that depends on a tag. I'm wondering whether we should align this more with the other images which just push whenever they were successful 🤔 WDYT @seldridge @mikeurbach?

Thanks, made a pull request at https://github.com/llvm/circt/pull/8500 that should allow us to get rid of CI changes in this pull request, if I'm not mistaken.

[towoe]

Hi, looks like the CI PR and rebase were successful. I did a rebase locally and have one more updated related to the newly added SVA test. https://github.com/towoe/circt/commit/4249e6ef3cf8a297814bae34626235d10d137948 Would be great if you, @hovind, could pick up this commit here.

Is there anything else still blocking this PR to go forward?

[likeamahoney]

Hi all, sorry to bother — what are the main issues preventing this from being merged?

[fabianschuiki]

Sorry for commandeering this PR, @hovind. Thanks for doing all the fantastic work to get this pretty much to the end :smiley:!

I have rebased and collapsed this PR onto the current main branch, and tweaked a few things. If anyone has a few spare cycles for a quick review, I'll land this :slightly_smiling_face:!

[towoe]

Awesome, thanks @hovind and @fabianschuiki and all the others to finally land this PR!

[hovind]

Sorry for commandeering this PR, @hovind. Thanks for doing all the fantastic work to get this pretty much to the end 😃!

I have rebased and collapsed this PR onto the current main branch, and tweaked a few things. If anyone has a few spare cycles for a quick review, I'll land this 🙂!

Sorry for my lack of responsiveness. I'm happy to see this landed, thanks for taking the reins! :tada: