[ImportVerilog] Bump slang
#7792
Conversation
CMakeLists.txt
Outdated
| FetchContent_MakeAvailable(slang) | ||
|
|
||
| set(CMAKE_CXX_STANDARD 20) | ||
| set(CMAKE_CXX_STANDARD_REQUIRED ON) |
There was a problem hiding this comment.
Not sure where it is most appropriate to set this, input would be much appreciated.
| else() | ||
| find_package(slang 3.0 REQUIRED) | ||
| find_package(slang 7.0 REQUIRED) |
There was a problem hiding this comment.
TODO: Ask slang to mint a release so that this version includes everything that we need, including MikePopoloski/slang#1164.
| inline bool operator==(uint64_t a, const FVInt &b) { return b == a; } | ||
| inline bool operator!=(uint64_t a, const FVInt &b) { return b != a; } | ||
| inline bool operator==(uint64_t a, const FVInt &b) { return b.operator==(a); } | ||
| inline bool operator!=(uint64_t a, const FVInt &b) { return b.operator!=(a); } |
There was a problem hiding this comment.
Any tips on how to best deal with https://timsong-cpp.github.io/cppwp/n4868/over.match#oper-3.4.4 in a codebase that still needs to support older standards?
|
Currently fails in CI due to GCC
as far as I can tell. clangThe |
| @@ -1,6 +1,4 @@ | |||
| # slang uses exceptions | |||
| set(LLVM_REQUIRES_EH ON) | |||
| set(LLVM_REQUIRES_RTTI ON) | |||
|
|
|||
| # For ABI compatibility, define the DEBUG macro in debug builds. Slang sets this | |||
| # internally. If we don't set this here as well, header-defined things like the | |||
There was a problem hiding this comment.
This really shouldn't be necessary; ideally adding the slang lib as a target should bring along its public compile definitions. Is there something I should be fixing on the slang side to do away with this here?
There was a problem hiding this comment.
Thanks a lot for taking a look here @MikePopoloski, it's much appreciated. I'm not good enough at CMake to answer all of your comments, but I'm hoping that somebody more qualified can chime in where needed.
| @@ -15,15 +13,10 @@ add_compile_definitions($<$<CONFIG:Debug>:DEBUG>) | |||
| if (MSVC) | |||
| # No idea what to put here | |||
| else () | |||
There was a problem hiding this comment.
Can you not specify slang include directories as being -isystem so that you don't need these warning suppressions?
| @@ -551,14 +551,17 @@ llvm_canonicalize_cmake_booleans(CIRCT_SLANG_BUILD_FROM_SOURCE) | |||
| if(CIRCT_SLANG_FRONTEND_ENABLED) | |||
| message(STATUS "slang Verilog frontend is enabled") | |||
| if(CIRCT_SLANG_BUILD_FROM_SOURCE) | |||
| # slang requires C++20 | |||
There was a problem hiding this comment.
Is this actually needed? slang's CMakeLists already sets these.
There was a problem hiding this comment.
I get an error trying to build ImportVerilog due to public headers in slang requiring C++ 20 if I remove this, but perhaps there's a more principled way to solve this?
There was a problem hiding this comment.
Ah I see, that makes sense. It's a little unfortunate that this is adding friction but I make extensive use of, e.g. std::span that would be hard to remove from the public headers. Hopefully LLVM / MLIR / CIRCT can upgrade to C++20 at some point across the board; it's coming up on 5 years old.
There was a problem hiding this comment.
Drive by comment: in general, it's OK to require c++20 (or other non-llvm-standard things like exceptions) if they're optional and can be disabled. Since slang is an optional dependency, it's OK to require c++20 when slang is being used. We just need to be sure to run a ci build with c++17 and no slang.
| @@ -573,6 +576,7 @@ if(CIRCT_SLANG_FRONTEND_ENABLED) | |||
| set(CMAKE_CXX_FLAGS "") | |||
There was a problem hiding this comment.
The MSVC /EHsc is probably not needed now?
| @@ -590,17 +593,24 @@ if(CIRCT_SLANG_FRONTEND_ENABLED) | |||
| # statically link slang into the CIRCTImportVerilog library, but seems to be | |||
| # harder than it ought to be. | |||
| set_property( | |||
There was a problem hiding this comment.
Yeah, it seems like all this install / export stuff shouldn't be needed but I'm not enough of a CMake expert to know offhand how to fix it.
| @@ -646,8 +646,8 @@ inline FVInt operator-(uint64_t a, const FVInt &b) { | |||
|
|
|||
| inline FVInt operator-(const APInt &a, const FVInt &b) { return FVInt(a) - b; } | |||
|
|
|||
| inline bool operator==(uint64_t a, const FVInt &b) { return b == a; } | |||
| inline bool operator!=(uint64_t a, const FVInt &b) { return b != a; } | |||
| inline bool operator==(uint64_t a, const FVInt &b) { return b.operator==(a); } | |||
There was a problem hiding this comment.
How is this related to the slang version? Is it picking up some overzealous global operator in slang somewhere?
There was a problem hiding this comment.
This is a consequence of building circt code with C++ 20 for the first time, where https://timsong-cpp.github.io/cppwp/n4868/over.match#oper-3.4.4 has come into play. If my analysis is correct, operator resolution of == in { return b == a; } where a: uint64_t and b: const FVInt &b resolved to inline bool operator==(uint64_t a, const FVInt &b) { return b == a; } , i.e. itself, leading to infinite recursion.
There was a problem hiding this comment.
I see, just an artifact of C++20 and not anything slang is doing. Maybe you can just ifdef out these overloads when compiling in C++20 mode.
| @@ -1,5 +1,5 @@ | |||
| // RUN: circt-verilog %s -E --verify-diagnostics | |||
| // REQUIRES: slang | |||
|
|
|||
| // expected-error @below {{could not find or open include file}} | |||
| // expected-error @below {{'unknown.sv': No such file or directory}} | |||
There was a problem hiding this comment.
FWIW this error message comes from the system / OS, so you may find this test fails on a different OS than the one you tested on.
There was a problem hiding this comment.
You've got a sharp eye, thanks!
There was a problem hiding this comment.
What do you think about return code checking?
hovind
force-pushed
the
dev/hovind/slang-master
branch
from
1d7fc88 to
c5631df
Compare
hovind
force-pushed
the
dev/hovind/slang-master
branch
from
02ed6b4 to
b538893
Compare
The `OBJECT` library target file generated by `llvm_add_library()` does not inherit `COMPILE_DEFINITIONS` from its transitive closure in the current implementation. To avoid generating this library, the condition `(ARG_SHARED AND ARG_STATIC) OR ARG_OBJECT` must not be satisfied. Neither `ARG_STATIC` nor `ARG_SHARED` is set at the moment, but `ARG_OBJECT` is satisfied due to `NEEDS_OBJECT_LIB` being set due to `NOT ARG_SHARED AND NOT ARG_EXCLUDE_FROM_LIBMLIR AND NOT XCODE AND NOT MSVC_IDE` being satisfied unless one of `ARG_SHARED`, `ARG_EXCLUDE_FROM_LIBMLIR`, `XCODE` or `MSVC_IDE` is satisfied.
Fixes operator overload resolution error on `gcc-11`.
Fixes operator overload resolution error on `gcc-11`.
hovind
force-pushed
the
dev/hovind/slang-master
branch
from
f4d4ae9 to
94f44fc
Compare
|
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. |
| @@ -40,8 +40,8 @@ jobs: | |||
| build-shared: [ON] | |||
| build-type: [Release] | |||
| compiler: | |||
| - cc: clang | |||
| cxx: clang++ | |||
| - cc: clang-17 | |||
There was a problem hiding this comment.
Please separate the build infrastructure changes from the rest of the PR.
|
|
||
| # Match the behavior of slang_slang, which installs its own vendored | ||
| # boost_unordered if it does not a system-wide boost installation. | ||
| find_package(Boost 1.82.0 QUIET) |
There was a problem hiding this comment.
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.
Some groundwork towards using a more up to date
slangverison.TODO
slang#7792 (comment).Figure out why f4d4ae9 is needed.Resolved in aca1839.