[ImportVerilog] Upgrade slang to slang 5

[teqdruid]

When slang was being added as a dep, the CI images couldn't handle C++20. They were very quickly upgraded to make them optionally use a newer clang version which supported C++20. The idea was to use slang 5 (which now supports compiling without RTTI and exceptions (IIRC) when a C++20 compiler was detected.

This hasn't happened yet.

[teqdruid]

@fabianschuiki How's this coming? (poke)

[astrophysik]

Hi, is there any work in progress to update the version of Slang used in CIRCT?

[fabianschuiki]

Not yet, but we should definitely try to do it! 😃 Since Slang is an optional feature of CIRCT, I don't think we have to care about the C++20 requirement too much. If users want Slang they'll have to have a C++20 compliant compiler. As long as our CI can run all the ImportVerilog tests, we're good 🥳

If you would like to give this a shot, try changing this line in CMakeLists.txt and see what kind of errors appear: https://github.com/llvm/circt/blob/main/CMakeLists.txt#L560

[teqdruid]

@fabianschuiki I spent a bunch of time getting the CI images to support C++20 when you first started (and I'm pretty sure I tested them with Slang 5) so they should be good.

[hovind]

Not yet, but we should definitely try to do it! 😃 Since Slang is an optional feature of CIRCT, I don't think we have to care about the C++20 requirement too much. If users want Slang they'll have to have a C++20 compliant compiler. As long as our CI can run all the ImportVerilog tests, we're good 🥳

If you would like to give this a shot, try changing this line in CMakeLists.txt and see what kind of errors appear: https://github.com/llvm/circt/blob/main/CMakeLists.txt#L560

I did some initial testing at 37597be, and the required changes to compile against the current master seem pretty minor. I haven't checked v5.0 yet.

ninja -C build/ check-circt currently outputs

Failed Tests (1):
  CIRCT-Unit :: Support/./CIRCTSupportTests/1/26

I'll take a look at that when I have time, unless somebody else beats me to it!

The most annoying thing I ran into was that they (as far as I can tell) require the input buffers to be zero terminated https://github.com/MikePopoloski/slang/blob/bad0d6ba3dec20355b0156c535bbddf2f926ab37/source/parsing/Lexer.cpp#L49, which means I failed to remove the extra copying described here.

[fabianschuiki]

That sounds very promising. Bumping this to v5 would certainly make @teqdruid very happy 😁

[hovind]

Any reason to prefer v5.0? If we want the llvm::SourceMgr to manage the input buffers, I think we'll need MikePopoloski/slang#1164, which has been merged after v7.0. I pushed an updated draft to main...hovind:circt:dev/hovind/slang-master.

[fabianschuiki]

I'd be very happy with version 7 as well! 👍

[teqdruid]

Ideally, we'd keep up with version bumps -- always stick to the latest.

…

On Fri, Nov 1, 2024, 3:36 PM Fabian Schuiki ***@***.***> wrote: I'd be very happy with version 7 as well! 👍 — Reply to this email directly, view it on GitHub <#7212 (comment)> or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALNXYFL2IFIZRBDJV4ZIQDZ6PJ37BFKMF2HI4TJMJ2XIZLTSSBKK5TBNR2WLJDUOJ2WLJDOMFWWLO3UNBZGKYLEL5YGC4TUNFRWS4DBNZ2F6YLDORUXM2LUPGBKK5TBNR2WLJDUOJ2WLJDOMFWWLLTXMF2GG2C7MFRXI2LWNF2HTAVFOZQWY5LFUVUXG43VMWSG4YLNMWVXI2DSMVQWIX3UPFYGLAVFOZQWY5LFVI3TCMBSGU4TEMJTGCSG4YLNMWUWQYLTL5WGCYTFNSWHG5LCNJSWG5C7OR4XAZNMJFZXG5LFINXW23LFNZ2KM5DPOBUWG44TQKSHI6LQMWVHEZLQN5ZWS5DPOJ42K5TBNR2WLKJSGQ2TANZSGUYTJAVEOR4XAZNFNFZXG5LFUV3GC3DVMWVDEMZWGI4TKMJZGA4YFJDUPFYGLJLMMFRGK3FFOZQWY5LFVI3TCMBSGU4TEMJTGCTXI4TJM5TWK4VGMNZGKYLUMU> . You are receiving this email because you authored the thread. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub> .

[astrophysik]

Is anyone doing this task? If not, I can try to do it

[hovind]

Is anyone doing this task? If not, I can try to do it

I've started a branch at #7792. Progress is slow, but I'm working on it every now and then when I find time.

It currently works on my machine, but I'm working on getting it through CI, as compiling with C++-20 and a new set of compilers has unearthed some new errors. There's also been an addition of a new boost_unordered dependency to slang, which has a COMPILE_DEFINITION. This COMPILE_DEFINITION is not propagated in llvm_add_library() when BUILD_SHARED_LIBS=ON, which is another thing that needs a proper solution.

[fabianschuiki]

The shared/static builds are definitely an adventure 🙈 I was originally hoping that we could statically link Slang into ImportVerilog regardless of whether CIRCT was built with dynamic or static libraries. No idea if that's reasonable or even achievable.

[astrophysik]

Progress is slow, but I'm working on it every now and then when I find time.

I'd like to try help if it possible

[hovind]

The shared/static builds are definitely an adventure 🙈 I was originally hoping that we could statically link Slang into ImportVerilog regardless of whether CIRCT was built with dynamic or static libraries. No idea if that's reasonable or even achievable.

As far as I understand, that is how it works today thanks to the set(BUILD_SHARED_LIBS OFF) before FetchContent_MakeAvailable(slang).

What I have been struggling with is that when llvm_add_library() decides to build a object library here, CMake generates the following build.ninja target:

build lib/Conversion/ImportVerilog/CMakeFiles/obj.CIRCTImportVerilog.dir/ImportVerilog.cpp.o: CXX_COMPILER__obj.2eCIRCTImportVerilog_unscanned_Debug /home/oystein/build/circt/lib/Conversion/ImportVerilog/ImportVerilog.cpp || cmake_object_order_depends_target_obj.CIRCTImportVerilog
  DEFINES = -DDEBUG -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
  DEP_FILE = lib/Conversion/ImportVerilog/CMakeFiles/obj.CIRCTImportVerilog.dir/ImportVerilog.cpp.o.d
  FLAGS = -fno-exceptions -fno-rtti -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 -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 -g -std=gnu++20  -fno-exceptions -funwind-tables -fno-rtti -Wno-non-virtual-dtor -Wno-ctad-maybe-unsupported -Wno-covered-switch-default
  INCLUDES = -I/home/oystein/build/circt/llvm/llvm/include -I/home/oystein/build/circt/llvm/build/include -I/home/oystein/build/circt/llvm/mlir/include -I/home/oystein/build/circt/llvm/build/tools/mlir/include -I/home/oystein/build/circt/include -I/home/oystein/build/circt/build/include -I/home/oystein/build/circt/build/_deps/slang-src/source/../include -I/home/oystein/build/circt/build/_deps/slang-build/source -I/home/oystein/build/circt/build/_deps/slang-src/source/../external -I/home/oystein/build/circt/build/_deps/fmt-src/include
  OBJECT_DIR = lib/Conversion/ImportVerilog/CMakeFiles/obj.CIRCTImportVerilog.dir
  OBJECT_FILE_DIR = lib/Conversion/ImportVerilog/CMakeFiles/obj.CIRCTImportVerilog.dir

compared to the target when an object library is not generated:

build lib/Conversion/ImportVerilog/CMakeFiles/CIRCTImportVerilog.dir/ImportVerilog.cpp.o: CXX_COMPILER__CIRCTImportVerilog_unscanned_Debug /home/oystein/build/circt/lib/Conversion/ImportVerilog/ImportVerilog.cpp || cmake_object_order_depends_target_CIRCTImportVerilog
  DEFINES = -DDEBUG -DGTEST_HAS_RTTI=0 -DSLANG_BOOST_SINGLE_HEADER -DSLANG_STATIC_DEFINE -D_DEBUG -D_GLIBCXX_ASSERTIONS -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
  DEP_FILE = lib/Conversion/ImportVerilog/CMakeFiles/CIRCTImportVerilog.dir/ImportVerilog.cpp.o.d
  FLAGS = -fno-exceptions -fno-rtti -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 -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 -g -std=gnu++20 -fPIC  -fno-exceptions -funwind-tables -fno-rtti -Wno-non-virtual-dtor -Wno-ctad-maybe-unsupported -Wno-covered-switch-default
  INCLUDES = -I/home/oystein/build/circt/llvm/llvm/include -I/home/oystein/build/circt/llvm/build/include -I/home/oystein/build/circt/llvm/mlir/include -I/home/oystein/build/circt/llvm/build/tools/mlir/include -I/home/oystein/build/circt/include -I/home/oystein/build/circt/build/include -I/home/oystein/build/circt/build/_deps/slang-src/source/../include -I/home/oystein/build/circt/build/_deps/slang-build/source -isystem /home/oystein/build/circt/build/_deps/slang-src/source/../external -isystem /home/oystein/build/circt/build/_deps/fmt-src/include
  OBJECT_DIR = lib/Conversion/ImportVerilog/CMakeFiles/CIRCTImportVerilog.dir
  OBJECT_FILE_DIR = lib/Conversion/ImportVerilog/CMakeFiles/CIRCTImportVerilog.dir

DEFINES for the object library object lacks -DSLANG_BOOST_SINGLE_HEADER -DSLANG_STATIC_DEFINE. These are not propagated in llvm_add_library().

I pushed a slightly less confusing hack to 88a8c02.

[hovind]

Progress is slow, but I'm working on it every now and then when I find time.

I'd like to try help if it possible

Fantastic!

Right now, the only failing CI failure is due to https://github.com/llvm/circt/blob/88a8c02fe3b75ad523c008154447c605eba6f31e/.github/workflows/shortIntegrationTests.yml running on ghcr.io/circt/images/circt-integration-test:v15.1, which does not have clang-17.

Feel free to help out wherever you would like, but a review of the cumulative changes in https://github.com/llvm/circt/pull/7792/files would be much appreciated.