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
CMake support #1363
Comments
Original Redmine Comment Nice patch set, you seem to have mastered the key stuff. Though not explicitly stated I assume your goal is to eventually get this merged upstream so here's some comments. First, please separate out the python and cmake patches (it seems python needs cmake though so would build on that patch), then when ready (below) As to the cmake patch my main concern is I haven't used CMake and when there are bugs a few years from now related to CMake are you willing to signup to support fixing them? The same applies to the python code, but here I need to soul search a bit as to if I want to accept it. This feels more of something for your personal use case and unlikely to be widely useful. Perhaps use it for at least a few months and see how it settles out. --- a/bin/verilator
Maybe it should be --make cmake (versus the default "--make gmake" similar to the "--compiler gcc" flag), so when yet another build system we don't need to keep adding unique flags?
+Lowercase and ... should be replaced with arguments, the uppercase parts delimit I think we should make the flag names close or identical to existing makefile arguments out of clarity. +=item NAME PREFIX? +=item TOP TOP_MODULE? +Optional. Sets the name of the top module. Defaults to the name of the first +=item ARGS VERILATOR_FLAGS? +=item INCLUDEDIRS INCLUDE_DIRS? but is this critical? Expect -f lists are the commoner case. +=item SLOW_FLAGS OPT_SLOW +=item FAST_FLAGS OPT_FAST +++ b/examples/cmake Example names should perhaps be cmake_c and python_c, since they assume C code generation. Please add comment headers similar to the existing example files. Call the written vcd file logs/vlt_dump.vcd to match tracing_c example. "make examples" on the top directory fails as no Makefile is found - think you need one just to run your steps documented. Following your instructions I get this: CMake Error at CMakeLists.txt:11 (cmake_policy): +++ b/examples/cmake/.gitignore Please also in gitignore include the files ignored in tracing_c/.gitignores. Fix some missing trailing newline. +++ b/examples/python/pyexample.cpp
Everywhere add one space after // or # comment chars. +++ b/include/verilated_py.h Indent #if blocks appropriately "# ifdef" etc. +namespace vl_py { VerilatedPy +inline void declare_globals(pybind11::module& m) {
Verilator doesn't currently require C++11, and this seems to. Perhaps it's ok to require C++ for this mode, but the test needs to be skipped when not configured for C++11. +++ b/src/V3EmitCBase.h
Indeed. Pushed this. +++ b/src/V3EmitCMake.cpp +static void cmake_set(std::ofstream& of, const string& name, const string& value,
Rather than pollute the global namespace, put these into your class. +static string quote(const string& s) { Quote is unused, remove. +static string normalise(string s) { Sorry, I'm American so "normalize". ;) Is non-windows happy with this? +++ b/verilator-config.cmake
I'm scared about having to maintain this code in two places (configure.ac and here). Not sure how to deal with that. Put the -'s in front of these as much better when grepping and some compilers use +'s.
You don't seem to check for the appropriate multithreaded flags. Need a test_regress/t/t_flag_cmake.pl test outside the examples. If your scenario line is right this will also test multithreaded. |
Original Redmine Comment That was quick. I'm actually off on holiday imminently, so won't get round to an updated patchset for a few weeks. The cmake and python bits are already in separate commits, though the python one depends on the cmake one, the cmake one works alone. I can focus on the cmake one in this issue if you like. Primarily I'm just punting this code out in the spirit of sharing. I would like to see it upstream, because it would make updating a lot easier for me, though it's completely understandable if you don't want to merge stuff you'll never use. As far as supporting it goes, I intend to keep it working for myself for the foreseeable future, and I might check on here occasionally or when I'm alerted, but I'm unlikely to add improvements that I don't need or respond to issues as quickly/comprehensively as you seem to. I might well be in a minority of one with the python stuff, though I think it's pretty good for directly comparing a reference implementation of some DSP algorithm written with numpy etc to a verilog implementation. I've been doing it manually for a while, and got sick of writing the boilerplate, so decided to make these patches. |
Original Redmine Comment Any movement on this Patrick? CMake is pretty popular, getting this in would make it so much easier to use Verilator in all kinds of other projects. |
Original Redmine Comment Hey I've rebased the cmake patch and updated it a bit. The code is at https://github.com/madebr/verilator/pull/1 changes:
questions/todos:
Things I did not change:
Greetings |
Original Redmine Comment Nice job.
Is what cmake ususally wants a "cmake" directory with all tools underneath it?
I think this should be examples/cmake_c to match the other examples (to
Given you call it build system I wonder if this should be --build, or you should say
Please also describe that the options are gmake, cmake or both.
Please capitalize Verilator and Verilated. Please add the cmake related files to the FILES documentation section.
Why is VERILATE_FLAGS made this way versus using the existing CFG_CXXFLAGS_NO_UNUSED?
Doesn't need to be templated. Move to inside V3EmitCMake class.
Use "it" instead of "i". Use "++it" instead of "it++". Use "out" instead of "s".
Likewise move inside V3EmitCMake.
Perhaps deslash instead of normalize? Input should be const string&.
I think there's a lot of duplication between here and V3EmitMk. Wonder if V3EmitMk should have a common
Nit, two spaces before any comment.
Should be able to be "V3FileDependImp::getAllDeps() const" and use a const_iterator. Ditto add const to related calling fuctions.
Use:
(The convention recently changed to quote things in messages).
Don't do this here as -f files will break, do it in Verilator.cpp after where it says e.g "Need -cc ...".
For these, please add a header of some sort to this, with Copyright and a DESCRIPTION at least.
Changes here seem reasonable. There's some commented out code you added,
This needs to skip() if there's no cmake installed or < version 3.8. Actually I think your example/cmake_c needs a similar check, for that see how SystemC is tested in the Makefile.
I think this is fine.
Not sure why this is. Are you sure cmake is installed when travis runs?
I would prefer to stick with OPT_FAST and OPT_SLOW; I searched for SLOW_FLAGS and this doesn't seem any more standard (this thread is the first google hit ;). Thanks, look forward to next patch set. |
Original Redmine Comment Thanks for taking this on Maarten, I haven't had time for a while.
|
Original Redmine Comment Hello Patrick, Thanks for the comments! |
Original Redmine Comment Hello, I think I have incorporated the comments in #5. They can still be found at https://github.com/madebr/verilator/pull/1 Because I've extracted the spelling fix to an external patch, I had to rebase and force push the series.
Greetings |
Original Redmine Comment I've created a new pull request in which I add myself to docs/CONTRIBUTORS to certify that this and future contributions are open sourced under the Developer Certificate of Origin |
Original Redmine Comment Thanks for all the cleanups. If you want just point me to a github branch and you can skip posting patch files.
Sorry, I'm not understanding how your change solves that problem. +++ b/bin/verilator
s/-system-system/-system/ +=item --make Use I before <> so pretty-prints nicely.
Please add trailing comment like other lines:
Think earlier feedback in V3EmitCMake about removing template and moving to static class functions was missed.
Please change to cmake_version, and have return floating version number which the caller then compares. (Some future new feature might require a larger version number and we'll be more ready.) When I run examples I get:
Note I do have VERILATOR_ROOT set. |
Original Redmine Comment If you're putting it into share/verilator/cmake, you need to set the parent directory of ${CMAKE_CURRENT_LIST_DIR} on line 28 of verilator-config.cmake.in. Wilson: You perhaps have cmake < 3.13 installed that doesn't support the -S and -B flags? Try: ```rm -rf build && mkdir build && cd build && cmake ..
|
Original Redmine Comment I'm using cmake 3.10.2. I'm running "make examples" inside the parent git repo checkout (not installed).
This worked, presumably please update the examples/cmake_c/Makefile to work appropriately, thanks. |
Original Redmine Comment Hello folks @patrick Stewart
@wilson Snyder
On my machine, the following flags are valid (by gcc)
By using the first list, cmake users will detect all flags.
Greetings |
Original Redmine Comment I pushed the contributors patch and typos patch - threw some other typo fixes in after searching for the same typo. Thanks for those. I think I understand now what you're saying about configure. My objection now is you are passing flags into cmake that were tested for reasons other than their needing to apply to verilated code, e.g. -Wno-null-conversion should not be used by cmake, and also you are using a low-level function that isn't intended to have side effects. I still think you should either use CFG_CXXFLAGS_NO_UNUSED, or have a new set of _MY_CXX_CHECK_OPT calls setting a new variable e.g. CFG_CXX_FLAGS_CMAKE even if that duplicates some checks. |
Original Redmine Comment I've rewritten |
Original Redmine Comment I've added a systemc example. I've tested this using Accellera's systemc. The test will only execute if systemc has been compiled with cmake support and if it has the same c++ standard as the verilated project. Also, because cmake should be the one in charge for finding the systemc include and library paths, Verilator does not need to output the systemc include and library paths in the generated cmake script. |
Original Redmine Comment Thanks, getting close. I still can't "make examples" out of the box.
Please revert to leave the split as-was, because I have scripts that look for DESCRIPTION and they shouldn't match this line.
Why should make install not also build?
I believe these are needed to make rpm/apt distros work, why did you remove them? Thanks for cleaning up configure.ac. Please add a comment
|
Original Redmine Comment Also please test that "make test" is clean with VERILATOR_AUTHOR_SITE=1 in your environment before you configure.
|
Original Redmine Comment
|
Original Redmine Comment I agree users should do a "make" before "make install" however I think it's a feature if someone changes code to make sure the build reoccurs on an install. I had Emacs and Bison sources around and they also rebuild on "make install" so I'll leave this.
I think you forgot to comment on this - I believe these are needed to make rpm/apt distros work, why did you remove them?
The t_a_first tests have this because sometimes Verilator crashes on the first test. Your test can remove these flags. (Just use "-sc") More critically this test doesn't seem to use "--make cmake". I merged your t_dist_manifest spell fix. |
Original Redmine Comment I agree to the
When using CMake, this include and library path is not needed because these paths are detected by CMake. When SystemC is configured, and installed using its CMake support, we are able to use its CMake scripts to link to SystemC (the same as users of Verilator would be able to use Verilator's CMake scripts). The SystemC example shows how this works: it contains a (Targets in CMake encapsulate information such as include directories, libraries, compile definitions, c++ standard, ...) These 2 functions are/were currently needed for emitting the Makefile. IMHO, when using CMake, Verilator should not try to do too much and let the user handle the SystemC paths.
The |
Original Redmine Comment Yes, the getenv stuff was added to reduce some questions I was getting about why SystemC would later break. I'll remove them in a separate patch so it's easier to bisect. By your philosophy I believe getenvSYSTEMC and getenvSYSTEMC_ARCH should also be removed? They didn't print errors so presumably that's why you left them.
Can you please have this print a reference to the Verilator documentation, and in the CMake section you added please add pointer to where to find instructions on how to get SystemC with CMake installed? Likewise if the user uses "verilator -sc" themselves then runs cmake without SystemC installed. Lots of people will hit this, so we need to make this quite clear. Thanks |
Original Redmine Comment
By "my philosophy", yes. When Verilator is only generating sources, it should not know about the location of a SystemC library. When specifying I've updated the example and the documentation. Currently, when the user Verilates a design, compiles and links, the compiler will complain about missing <systemc.h> and a linking error. That's why I've added a convenience function I hope that suffices to avoid an influx of build problems.. I'm working on the Python support now to be sure that the CMake scripts work as designed. |
Original Redmine Comment Thought we were done... but not quite. Can you please change to use these variables: SYSTEMC_INCLUDE=/example/path/to/systemc-2.3.3/usr/include There's two problems I'm having with SYSTEMC_ROOT, first we didn't require it previously so it isn't backward compatible, and second in some systems such as one I use you can't assume it's just SYSTEMC_ROOT/include and SYSTEMC_ROOT/lib. FWIW I agree SYSTEMC_ROOT is a better name. I'd suggest one of three options:
|
Original Redmine Comment Hello, I've gone for option 3: First look in SYSTEMC_INCLUDE/SYSTEMC_LIBDIR, then try SYSTEMC_ROOT, then try SYSTEMC and then try using a CMake target provided by a SystemC installation. This order is only relevant when a user uses the verilator_link_systemc convenience function. So I've added this documentation only to the CMake section. It's not finished yet, but I've pushed pushed python support to the python branch in my fork. |
Original Redmine Comment The sc example now works, thanks.
CMake Warning at CMakeLists.txt:122 (find_package): With those fixed I will merge into trunk. This bug has gotten quite long and with the cmake merge I'd like to be able to close it, so can you please file a new one for python support? Thanks. |
Original Redmine Comment I have applied your patch and think I have fixed the problem with t_flag_make_cmake_sc.pl I would like to delay merging this patch to master until after we have stabilized Python support using CMake because I'm not 100% sure all assumptions are valid. For example, the current CMake script did unconditionally set VL_PRINTF=printf. Soon, I'll open a new bug report for the Python support (current work is in python branch). |
Original Redmine Comment Ok, I'll hold off merging. If you could please merge from master into cmake, then merge cmake into your python branch. I can then in the future do a cmake..python diff to see your python changes. |
Original Redmine Comment I've added a rebased version as a pull request on your new(?) github repo. I'm hoping we can get the CMake in at least, let me know if there's any issues. |
Original Redmine Comment Principally it looks like the tests where it compares the STDOUT to a golden file fail. I propose to just add a cmake_expect and cmake_expect_filename similar to what's done for other simulators, so that part of the test will effectively be skipped. Alternatively for systemc, rather than trying to parse out a compiler dependent C++ standard flag we could just add $ENV{SYSTEMC_CXX_FLAGS} to the compiler flags and avoid setting any C++ standard in CMake if we're in systemc mode. Then people have the option to set a C++ standard of their choice using SYSTEMC_CXX_FLAGS or manually in their own CMakeLists.txt. |
Original Redmine Comment Patrick Stewart wrote:
I have implemented the latter. If we use the SystemC library pointed by environment or cmake variables, we use the SYSTEMC_CXX_FLAGS cmake or environment variable. So the SYSTEMC_CXX_FLAGS environment variable on the test system must contain "-std=c++14" |
Original Redmine Comment
I'd prefer not to have expecations be different, instead tests should be different - I'm not intending that you commit running everything under cmake, so this is not needed. If there are bugs you find in this process, consider extending the cmake test_regress or adding a new test_regress instead, then the normal "expect" will suit the job. |
Original Redmine Comment Passing SYSTEMC_CXX_FLAGS will still fail the vltmt test because the C++11 flag of multithreaded will overrule the std=c++XX of SYSTEMC_CXX_FLAGS (because it is the latest compiler argument). I am still in favour of https://www.veripool.org/issues/1363-Verilator-CMake-support#note-45 because that feels more like CMake. |
Original Redmine Comment I suggest we should not set any language flags, I think any other policy will just lead to trouble with libraries. If the user gives SYSTEMC_CXX_FLAGS then those would be added as usual. Note the current "gmake" tests do NOT use CFG_CXXFLAGS_STD_NEWEST nor CFG_CXXFLAGS_STD_OLDEST except in a special test. |
Original Redmine Comment I was thinking we'd change the condition around setting C++11 to if(THREADING && !SYSTEMC) rather than if(THREADING). The cases are:
In either case threading works if systemC library is >= C++11, otherwise it can't work whatever we do. EDIT: Didn't see Wilson's post, I'm happy to go with that option |
Original Redmine Comment I think we should assume C++11, so not muck with flags. At present Verilator complains on any non-C11 system and says to contact us, and also says threading won't work. I know of only one site in that camp, and expect to cut them off next year (e.g. always require C++11 even single threaded). |
Original Redmine Comment Ok, cmake support will require the user to set the correct C++ standard and pass the correct SYSTEMC_CXX_FLAGS. It will also ignore the C++ standard, set by the SystemCLanguage's find_package. This will not work on older compilers that have a default c++ version less then 11. This fails on travis-ci. (I'm still refering to #45) Patrick, can you check my patstew_cmake branch? (There is a small fix in driver.pl) Wilson Snyder wrote:
This is not completely correct. The verilator tests will append CFG_CXXFLAGS_STD_NEWEST when threaded (in verilator.mk) |
Original Redmine Comment Ok, I've made a few more tweaks. Most of the tests work now with (despite) this patch:
The ones that don't are either looking at the wrong log or using make_flags which cmake ignores, so I think they can be ignored. There's a commit at the end of the cmake pull which is just some minor tweaks to the test suite that fixes tests on cmake and shouldn't hurt the real tests. |
Original Redmine Comment Great. Thanks for separating, I pushed the test suite change part. Looking at rest... |
Original Redmine Comment Maarten is right, without setting the C++ version threading fails on the Travis compiler. I've gone back to if(THREADING && !SYSTEMC) then C++11. |
Original Redmine Comment Do you think this is ready to merge to master? Took PATSTEW/cmake, merged master (which shouldn't affect this though) and got this: v4make test_regress/t/t_a4_examples.pl --dist
|
Original Redmine Comment Yes, I think so. I thought I'd changed that to be compatible with earlier versions of cmake, but the bit that builds both shared and static libraries still needs 3.12 as it turns out, so I've reverted the version check in the Makefile. If you want to test it with CMake <3.12 you can follow the comments in examples/cmake_protect_lib/CMakeLists.txt to only build the static library. Or replace it with:
|
Original Redmine Comment I am using Ubuntu 18.04 LTS which is the most recent LTS of Ubuntu. This has CMake 3.10.2 out-of-the-box. I don't see how we can realistically expect people to have a newer version than this. (People won't want to/know how to upgrade their CMake to something beyond what is in the latest standard distro.) |
Original Redmine Comment Well it's only the protect_lib example. How about I flip the default of that file so it only builds a static library but leave a comment about how to build both? Or maybe building both simultaneously isn't so important when it's so easy to change and I should just simplify it to static only? |
Original Redmine Comment Sorry, I'm not completely up to date on this thread. But regarding static/shared libraries for --protect-lib, this is meaningful as some non-Verilator simulators require the shared library. It's not critical for the example project as that is Verilator-only. But it is needed for t_prot_lib.pl (as I test against other simulators) and for using the feature in general. Am I understanding correctly that there is something about building shared libraries that is not supported until CMake 3.12? Or is it an issue with building both the static and shared versions of the library at the same time? |
Original Redmine Comment It's just the specific way I've used to build both simultaneously here that needs >=3.12. Building one or the other is just s/STATIC/SHARED/. You can also build both by simply copying the same thing out twice on older versions of CMake. |
Original Redmine Comment I've pushed another version that defaults to just building a static lib, that should work with older versions of CMake. You'll probably still get the "You have called ADD_LIBRARY for library verilated_secret_obj without any source files." warning, but it's spurious and shouldn't be fatal. |
Original Redmine Comment I've added two trivial documentation changes after discussion with Maarten on github. |
Original Redmine Comment
%Warning: vlt/t_flag_make_cmake: obj_vlt/t_flag_make_cmake/CMakeCache.txt does not exist |
Original Redmine Comment As in you ran "make test_regress" in the main directory and got that error? Any more context for it? Did CMake print any errors? I've run:
and all tests pass for me except @t_prot_lib_inout_bad@ where the log doesn't match (the @v3fatalSrc@ on line 358/359 of @V3ProtectLib.cpp@ is split across both lines and GCC 9.1 gives @LINE@ as 358 whereas the golden file has 359). I've also had some random failures that I think are caused by @t_prot_lib@ running @t_prot_lib_secret@ while @t_prot_lib_secret@ is already running on its own. |
Original Redmine Comment Thanks for all your work on this. This is pushed to git towards eventual 4.022 release. I'm sure you'll have tweaks, please file a new issue/pull request for them. |
Original Redmine Comment In 4.022. |
It doesn't seem like the Python features got merged into master? |
Author Name: Patrick Stewart
Original Redmine Issue: 1363 from https://www.veripool.org
Original Assignee: Patrick Stewart
I've added support for building with CMake (the verilated output, not verilator itself), and for generating a python module as output using pybind11. The code is here: https://github.com/patstew/verilator/tree/python
I've added documentation and examples for each, hopefully they're clear enough.
The text was updated successfully, but these errors were encountered: