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

[ImportVerilog] Update to Slang 4 #6629

Closed
wants to merge 1 commit into from
Closed

Conversation

fabianschuiki
Copy link
Contributor

Bump the Slang dependency to version 4. This version supports builds with exceptions and RTTI fully disabled, which allows us to get rid of the ugly glue code that tried to bridge the gap between the RTTI/EH world of Slang and the non-RTTI/non-EH world of LLVM.

@fabianschuiki fabianschuiki added the Verilog/SystemVerilog Involving a Verilog dialect label Jan 31, 2024
@fabianschuiki
Copy link
Contributor Author

@teqdruid This Slang version bump gets rid of the RTTI/EH issues. Last time I run this though I was having trouble with the older compiler versions present in the CI image. You mentioned you might be willing to look into updating them? I'm also not entirely sure how to get this to build with MSVC... maybe you have some insight 🤔

@fabianschuiki fabianschuiki force-pushed the fschuiki/update-slang branch 2 times, most recently from c3d2a14 to eaeaa9b Compare January 31, 2024 00:57
@darthscsi
Copy link
Contributor

hmm, depending on concepts, that's quite a jump.

@fabianschuiki
Copy link
Contributor Author

Yeah ☹️ I guess things will eventually land and become stable, but it also limits deployability of Slang and CIRCT. In the worst case we can always fork Slang 3 and add the necessary tweaks to make it non-RTTI/non-EH.

@teqdruid
Copy link
Contributor

I'm finally working on this. Seems to be very sensitive to both clang and gcc versions (even though they have both "supported" c++20 for some time). We should add version checks to prevent random build failures. Still trying to determine the minimum versions.

@fabianschuiki
Copy link
Contributor Author

How do folks in general feel about modern C++ versions? As in, do we care if CIRCT requires a fairly modern compiler, with the benefit of using newer Slang versions, or do we care about CIRCT being easy to deploy on compilers that are older than a year, with the downside of older Slang versions and having to do some manual work to get exceptions out?

@teqdruid
Copy link
Contributor

How do folks in general feel about modern C++ versions? As in, do we care if CIRCT requires a fairly modern compiler, with the benefit of using newer Slang versions, or do we care about CIRCT being easy to deploy on compilers that are older than a year, with the downside of older Slang versions and having to do some manual work to get exceptions out?

I have no problem with optional parts of CIRCT requiring newer versions of compilers, so long as we check. I do, however, feel somewhat strongly that the non-optional stuff should compile on older compilers. A lot of people (including myself) still use Ubuntu 20.04, which has pretty ancient versions out-of-the-box. This is especially true if we ever want to upstream anything.

Back porting the new no RTTI/EH features from slang seems like a bad idea to me. We'd be stuck on an older version without any new features!

@fabianschuiki
Copy link
Contributor Author

That makes sense. How would we go about ensuring that everything works on older compilers? Would we add a CI job that runs on some minimum version of Ubuntu that builds with the non-optional parts? It would be great if we had regular CI with all stuff enabled, such that it is properly checked; but that also requires us to have usual CI run on very new compilers, which just asks for the backwards compatibility to slowly rot away if we don't explicitly check for it.

@teqdruid
Copy link
Contributor

That makes sense. How would we go about ensuring that everything works on older compilers? Would we add a CI job that runs on some minimum version of Ubuntu that builds with the non-optional parts? It would be great if we had regular CI with all stuff enabled, such that it is properly checked; but that also requires us to have usual CI run on very new compilers, which just asks for the backwards compatibility to slowly rot away if we don't explicitly check for it.

The CI images now run Ubuntu 20.04. I'm in the process of updating the integration image to the minimum versions (clang 16 and gcc 10, since clang uses g++'s stdlib) so we'll at least be able to test import verilog there. This is what I do for the ESI runtime -- it's only tested in the integration tests, both the short one which runs on PRs and the nightlies. In general, it's the only pipeline which tests everything.

Do you want import verilog to run on the standard pipelines?

@teqdruid
Copy link
Contributor

Building with clang requires clang-16 and g++-10 (since clang uses g++'s stdlib).

@teqdruid
Copy link
Contributor

@fabianschuiki For GCC, g++-11 seems to get through slang, but fails with:

Building CXX object tools/circt/lib/Conversion/ImportVerilog/CMakeFiles/obj.CIRCTImportVerilog.dir/ImportVerilog.cpp.o
FAILED: tools/circt/lib/Conversion/ImportVerilog/CMakeFiles/obj.CIRCTImportVerilog.dir/ImportVerilog.cpp.o
/usr/bin/g++-11 -DDEBUG -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/_circt_build/tools/circt/lib/Conversion/ImportVerilog -I/circt/lib/Conversion/ImportVerilog -I/_circt_build/include -I/circt/llvm/llvm/include -I/circt/include -I/_circt_build/tools/circt/include -I/_circt_build/_deps/slang-src/source/../include -I/_circt_build/_deps/slang-build/source -I/_circt_build/_deps/slang-src/source/../external -I/_circt_build/_deps/fmt-src/include -isystem /circt/llvm/llvm/../mlir/include -isystem /_circt_build/tools/mlir/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -fno-lifetime-dse -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-uninitialized -Wno-nonnull -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -g -fvisibility-inlines-hidden  -fno-exceptions -funwind-tables -fno-rtti -Wno-non-virtual-dtor -Wno-c++98-compat-extra-semi -Wno-ctad-maybe-unsupported -Wno-cast-qual -Wno-covered-switch-default -std=c++17 -MD -MT tools/circt/lib/Conversion/ImportVerilog/CMakeFiles/obj.CIRCTImportVerilog.dir/ImportVerilog.cpp.o -MF tools/circt/lib/Conversion/ImportVerilog/CMakeFiles/obj.CIRCTImportVerilog.dir/ImportVerilog.cpp.o.d -o tools/circt/lib/Conversion/ImportVerilog/CMakeFiles/obj.CIRCTImportVerilog.dir/ImportVerilog.cpp.o -c /circt/lib/Conversion/ImportVerilog/ImportVerilog.cpp
In file included from /_circt_build/_deps/slang-src/source/../include/slang/text/SourceLocation.h:14,
                 from /_circt_build/_deps/slang-src/source/../include/slang/diagnostics/Diagnostics.h:18,
                 from /_circt_build/_deps/slang-src/source/../include/slang/diagnostics/DiagnosticEngine.h:16,
                 from /_circt_build/_deps/slang-src/source/../include/slang/diagnostics/DiagnosticClient.h:10,
                 from /circt/lib/Conversion/ImportVerilog/ImportVerilog.cpp:23:
/_circt_build/_deps/slang-src/source/../include/slang/util/Hash.h:19:14: fatal error: boost/unordered/unordered_flat_map.hpp: No such file or directory
   19 | #    include <boost/unordered/unordered_flat_map.hpp>

@teqdruid
Copy link
Contributor

Actually, that error occurs even in the clang build... wonder how I got through it in my test builds.

@fabianschuiki
Copy link
Contributor Author

Let me rebase this onto master. Probably something broken since we've added circt-verilog preprocessing.

@teqdruid
Copy link
Contributor

I've confirmed that the slang_slang target builds with clang-16 and gcc-11. I've (tried to) enable slang builds in the CI workflow for gcc builds only. This'll probably still break the nightly matrix test (since it doesn't know about the compiler requirements) and gcc-11 isn't in the integration_test image. Will add it now to take care of the latter problem.

@teqdruid
Copy link
Contributor

I've pushed the new images into main: 017bf54

@fabianschuiki
Copy link
Contributor Author

Awesome 🥳 Thanks a lot for the help! I'll see what I can do about the Slang build. There are a few weird things going on related to the packaged version of boost_unordered. Some defines are not propagated to CIRCTImportVerilog, which then breaks when some Hash.h file in Slang tries to pull in boost_unordered.

Bump the Slang dependency to version 4. This version supports builds
with exceptions and RTTI fully disabled, which allows us to get rid of
the ugly glue code that tried to bridge the gap between the RTTI/EH
world of Slang and the non-RTTI/non-EH world of LLVM.

Co-authored-by: John Demme <[email protected]>
@fabianschuiki
Copy link
Contributor Author

Closing this in favor of @hovind's effort in #7792.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Verilog/SystemVerilog Involving a Verilog dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants