Python models #3

patstew · 2019-10-04T19:37:08Z

This is the python support (on top of the CMake patches in #2) as discussed in issue #1489. I have added support for coverage, and made a couple of small fixes on top of Maarten's most recent work.

wsnyder · 2019-10-04T21:45:49Z

As described in https://github.com/verilator/verilator/blob/master/docs/CONTRIBUTING.adoc
please certify your source-code contributions are open source. Either:

** Have your patch include the addition of your name to docs/CONTRIBUTORS. (This was missing, so the regression failed).
** Or, use "git -s" as part of your commit.
** Or, email, or post a statement that you certify your contributions.

Thanks

madebr · 2019-10-04T22:59:16Z

@wsnyder I should already be in the CONTRIBUTORS list.

@patstew Maybe test the coverage addition?
I should also say that I completely rewrote src/V3EmitPy.cpp in my pydpi branch.
This pydpi branch is not merge worthy yet as the python dpi example should be converted into a regression test.

wsnyder · 2019-10-04T23:12:52Z

@madebr, you are certified, but if you're going to continue to use Anonymous Maartin rather than your other name, let me know as I'll also add that as otherwise it breaks this auto-test.

Need a certification from @patstew.

https://github.com/verilator/verilator/pull/3/checks?check_run_id=248461150
dist/t_dist_contributors: %Warning: dist/t_dist_contributors: Certify your contribution by appending 'Patrick Stewart' to CONTRIBUTORS
dist/t_dist_contributors: %Error: Certify your contribution by appending 'Anonymous Maarten' to CONTRIBUTORS

madebr · 2019-10-04T23:14:05Z

@wsnyder I'll git config change my name for this repo.

patstew · 2019-10-07T17:10:18Z

I've just pushed a new version that addresses the issues raised on the bug tracker. I've renamed all Maarten's contributions to match his name in CONTRIBUTORS, I hope that's OK @madebr.

patstew · 2019-10-07T21:51:06Z

This should have a green light on travis now, I've fixed the test failures.

madebr · 2019-10-08T15:43:15Z

The cmake files in the python pr should also be modified to use 2 spaces.
(in examples and include/verilated_py.cmake)

You can btw squash my 3 python patches (and modify the author email address, the same with my patch in the cmake pr)

madebr · 2019-10-09T12:26:44Z

include/verilated_py.cpp

@@ -41,41 +41,34 @@ namespace py = pybind11;
 namespace vl_py {
 namespace impl {
 py::int_ read_port(const vluint32_t* words, size_t width, bool is_signed) {
-    const size_t nb_words = (width + 31) / 32;
-    const size_t nb_bytes = (width + 7) / 8;
-    vluint8_t bytes[nb_bytes];


An alternative can be to use

std::vector<vluint8_t> buffer; buffer.resize(nb_bytes);

I checked PyPy has _PyLong_FromByteArray since version 5.0.
But granted, I don't know the performance impact.
I believe encryption modules will be mostly impacted by this.

Yea, I needed to get rid of the VLA because MSVC doesn't support them. The other issue is that if you have a signed integer that's not a multiple of 8 bits, then you'll get a positive number with _PyLong_FromByteArray because verilator doesn't sign extend. I suppose that could be fixed the same as I've done for small integers though. Maybe I'll switch it back.

My performance claim is totally unfounded. I don't know whether this has impact.

Apparently, I did not check for this sign problem.
I think a test_regress would be useful.

veripoolbot · 2020-04-21T01:31:18Z