-
Notifications
You must be signed in to change notification settings - Fork 299
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] Bump slang
#7792
base: main
Are you sure you want to change the base?
Changes from 9 commits
8ec44a2
a2e1237
e2e653d
f23891a
bcf2c79
64f339d
9fe0c5f
97b37dc
8df12d5
0e57ecc
6409c40
4268ba1
c5631df
01c5844
4f79b7f
1695cdf
0e128b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
set(CMAKE_CXX_STANDARD 20) | ||
set(CMAKE_CXX_STANDARD_REQUIRED ON) | ||
|
||
# Build slang as part of CIRCT (see https://sv-lang.com/building.html) | ||
message(STATUS "Building slang from source") | ||
include(FetchContent) | ||
FetchContent_Declare( | ||
slang | ||
GIT_REPOSITORY https://github.com/MikePopoloski/slang.git | ||
GIT_TAG v3.0 | ||
GIT_SHALLOW ON) | ||
GIT_TAG bad0d6ba3dec20355b0156c535bbddf2f926ab37) | ||
set(FETCHCONTENT_TRY_FIND_PACKAGE_MODE "NEVER") | ||
|
||
# Force Slang to be built as a static library to avoid messing around with | ||
|
@@ -573,6 +576,7 @@ if(CIRCT_SLANG_FRONTEND_ENABLED) | |
set(CMAKE_CXX_FLAGS "") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The MSVC /EHsc is probably not needed now? |
||
endif () | ||
set(BUILD_SHARED_LIBS OFF) | ||
set(SLANG_USE_MIMALLOC OFF) | ||
FetchContent_MakeAvailable(slang) | ||
|
||
set(CMAKE_CXX_FLAGS ${ORIGINAL_CMAKE_CXX_FLAGS}) | ||
|
@@ -581,7 +585,6 @@ if(CIRCT_SLANG_FRONTEND_ENABLED) | |
if(BUILD_SHARED_LIBS) | ||
set_target_properties(slang_slang PROPERTIES POSITION_INDEPENDENT_CODE ON) | ||
set_target_properties(fmt PROPERTIES POSITION_INDEPENDENT_CODE ON) | ||
set_target_properties(unordered_dense PROPERTIES POSITION_INDEPENDENT_CODE ON) | ||
endif() | ||
|
||
# The following feels *very* hacky, but CMake complains about the | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
GLOBAL APPEND PROPERTY CIRCT_EXPORTS slang_slang unordered_dense fmt) | ||
GLOBAL APPEND PROPERTY CIRCT_EXPORTS slang_slang fmt) | ||
|
||
# Disable the installation of headers coming from third-party libraries. We | ||
# won't use those APIs directly. Just make them static libraries for the sake | ||
# of running slang normally. | ||
set_target_properties(fmt PROPERTIES PUBLIC_HEADER "") | ||
set_target_properties(unordered_dense PROPERTIES PUBLIC_HEADER "") | ||
|
||
install(TARGETS slang_slang unordered_dense fmt EXPORT CIRCTTargets) | ||
install(TARGETS slang_slang fmt EXPORT CIRCTTargets) | ||
|
||
# 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) | ||
if(NOT Boost_FOUND) | ||
set_property(GLOBAL APPEND PROPERTY CIRCT_EXPORTS boost_unordered) | ||
install(TARGETS boost_unordered EXPORT CIRCTTargets) | ||
endif() | ||
else() | ||
find_package(slang 3.0 REQUIRED) | ||
find_package(slang 7.0 REQUIRED) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: Ask |
||
endif() | ||
endif() | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a consequence of building There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
inline bool operator!=(uint64_t a, const FVInt &b) { return b.operator!=(a); } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
|
||
inline raw_ostream &operator<<(raw_ostream &os, const FVInt &value) { | ||
value.print(os); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks a lot for taking a look here @MikePopoloski, it's much appreciated. I'm not good enough at |
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you not specify slang include directories as being -isystem so that you don't need these warning suppressions? |
||
# slang uses exceptions; we intercept these in ImportVerilog | ||
add_compile_options(-fexceptions) | ||
add_compile_options(-frtti) | ||
# slang has some classes with virtual funcs but non-virtual destructor. | ||
add_compile_options(-Wno-non-virtual-dtor) | ||
# some other warnings we've seen | ||
add_compile_options(-Wno-c++98-compat-extra-semi) | ||
add_compile_options(-Wno-ctad-maybe-unsupported) | ||
add_compile_options(-Wno-cast-qual) | ||
# visitor switch statements cover all cases but have default | ||
add_compile_options(-Wno-covered-switch-default) | ||
endif () | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You've got a sharp eye, thanks! |
||
`include "unknown.sv" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually needed? slang's CMakeLists already sets these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get an error trying to build
ImportVerilog
due to public headers inslang
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.