[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!
hovind
force-pushed
the
dev/hovind/slang-master
branch
from
1d7fc88
to
c5631df
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`.
Some groundwork towards using a more up to date
slangverison.