[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.