Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-08-29T22:36:24Z

the Python compile definition is VL_PYTHON. Is this ok? Because some variables have a VM_ prefix and some VL_.

VM_ means the variable is defined in the Makefile. VL_ means defined elsewhere.

the Python bindings allow users to set and read ports. It is currently not
possible to fetch metadata of modules. Would that be useful? I know how to
get the width of ports but is it possible to get parameters of modules?

I can see that this might be useful but expect it would rarely be used so
would delay implementing this. Note parameter information is passed only
if the parameter was marked verilator public.

The cpu usage should only rise when the computations start and lower again
when it is finished.

Agreed, this should be fixed.

Python does not have ani VPI/DPI support, is that needed? Or is that SystemC only?

The VPI probably isn't that useful. Allowing DPI calls from Verilog into
python, or python into Verilog seems useful but might be a lot of work to
implement.

Would support for verilator_coverage be useful?

I assume you mean the calls needed to save coverage. I would think so.

Original Redmine Comment
Author Name: Patrick Stewart
Original Date: 2019-10-04T19:47:33Z

I've rebased this work onto master, added a couple of minor fixes and added coverage support here: #3

The CPU usage bug is fixed here: #1

In any case it's not really related to the python support, so it shouldn't be a blocker IMHO.

I think that covers all the outstanding issues? It's working well for me in it's current state, I assume the same is true for Maarten?. I'm hoping we can get this in so I don't have to rebase it anymore :).

Original Redmine Comment
Author Name: Maarten De Braekeleer
Original Date: 2019-10-04T23:03:17Z

I have added DPI support for Python in a pydpi branch.
These commits should go on top of the python branch.
(which is equivalent to the pull request by Patrick Stewart)

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-04T23:20:43Z

Note to self this is also #3

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-05T01:13:19Z

I'm getting confused by all the layers. Do you propose

1. cmake goes to master, 2. everything in pydpi?

Or 1. cmake, 2. python no pydpi, 3. pydpi?

If you want to add pydpi separately, file a separate bug and we'll get
through the python support first, then deal with that. Otherwise a bit
more to review at once, but can do it that way too.

Anyhow here's combined feedback:

First, this is cool. Once we get this merged, I'd love to see a conference
presentation on using this (standalone, as opposed to burried under Cocotb,
which is also of course presentation worthy).

I saw some merge conflicts with trunk. Patrick's comment suggested he had
rebased, so that was a bit surprising, please merge with master.

 +++ b/README.pod
      examples/cmake_c            => Example building hello_world_c with CMake
      examples/cmake_sc           => Example building hello_world_sc with CMake
 +    examples/python_cmake       => Example verilog->Python conversion with CMake
 +    examples/python_make        => Example verilog->Python conversion with make

To match other examples, think should be called make_python, and cmake_python.
Also add you new ones using similar naming (cmake_python_dpi) and add to this list.

 +=item --python
 +
 +This generates a file wraps the toplevel module into a Python module using
 +pybind11 (https://github.com/pybind/pybind11). Python support is only available
 +for C++ output without SystemC. It is recommended to use CMake to build
 +Python modules.

Generate a Python module for the design by creating a wrapper using
pybind11 (https://github.com/pybind/pybind11). Python support is only
available for C++ output, not using SystemC. It is recommended to use CMake
to build Python modules BECAUSE WHY?

 +++ b/examples/python_cmake_dpi/dpi_main.py
 +    def dpii_f_bit(cls, i: Vtop.svBitVecVal):
 +        res = ~i
 +        print("in dpii_f_bit", i, res)
 +        return res

Thanks for making a relatively complete DPI test. Please move it into
test_regress. Then take this good example and shrink it to include only
what is critical; as is it is now I think it'll be too confusing for people
to figure out. Also heavily comment.

(Please also make sure anything tested in examples is also in test_regress,
examples aren't test cases.)

 +++ b/include/verilated.h

 +#ifdef VL_PYTHON
 +#include <cstdarg>
 +#ifdef VL_PRINTF
 +//#warning "VL_PRINTF was already defined. Undefining..."
 +#undef VL_PRINTF
 +#endif
 +#ifdef VL_VPRINTF
 +//#warning "VL_VPRINTF was already defined. Undefining..."
 +#undef VL_VPRINTF
 +#endif

Please indent the directives appropriately.

Why do you need to undef VL_PRINTF?

 +++ b/include/verilated.h
 -    static void commandArgsAdd(int argc, const char** argv);

Why being removed?

 +++ b/include/verilated.mk.in

 +#######################################################################
 +##### Python builds
 +
 +ifeq ($(VM_PYTHON),1)
 +  CPPFLAGS += $(shell python3-config --includes) -fPIC -DVL_PYTHON -DVL_USER_FINISH -DVL_USER_STOP -DVL_USER_FATAL
 +  LDFLAGS  += $(shell python3-config --ldflags) --shared
 +endif

Based on my experience with Verilog-Perl, expect that the includes & ldflags
reported by python will be incorrect on a lot of systems and will require tweaking
especially on MacOS. I don't think you need to do anything, just a heads-up.

 +++ b/include/verilated_py.cpp
 +++ b/include/verilated_py.h

I need to review these more closely.

Please use // comments instead of /**/ style, unless this breaks pybind (really?).

 +++ b/src/V3AstNodes.h
 -    string      m_rtnType;              // void, bool, or other return type
 +    string      m_rtnTypeString;        // String representation of return type
 +    AstBasicDTypeKwd m_rtnType;         // Return type
 +    bool        m_context:1;            // Context function

Ok, I guess having this is reasonable and cleaner than how it was
previously. Please make a separate patch off master to add this & related
changes in for this.

 +++ b/src/V3EmitMk.cpp
 +#include <memory>

What needs memory?

 +++ b/src/V3EmitMk.cpp
 +            of.puts("default: "+v3Global.opt.prefix()+"__PYTHON\n");

All caps seem ugly unless that's a standard.

 +++ b/src/V3EmitPy.cpp

Also too much for me to review at the moment.

There's a lot of code here, please see if there's any overlap with the
normal C output that could be common functions.

 +++ b/src/V3File.cpp
          while (isspace(*cp)) cp++;
          switch (*cp) {
 +        case '\0':  // End of string... No need for whitespace before it
 +            return (0);

 -        putsNoTracking(indentStr(endLevels(strg)));
 +        putsNoTracking(indentSpaces(endLevels(strg)));

Good stuff. indetStr should have been killed a while ago. I pushed this.

 @@ -706,11 +708,11 @@ void V3OutFormatter::puts(const char *strg) {
 -            if (cp[1]=='\0') {
 +            if (cp[1]=='\0' || cp[1]=='\n') {
                  m_prependIndent = true;  // Add the indent later, may be a indentInc/indentDec called between now and th$

Need to think about this, what did it fix? If you have any other
indent/similar changes that are outside python support, let's get them in
first & separately.

 +++ b/test_regress/t/t_flag_make_cmake_python.pl

Normally yes, this would be the exactly correct naming for a new flag's
test. In this case python is something pretty large/important, and we'll
end up with other tests added that are python-related, so suggest maybe
t_python_cmake.pl.

 +++ b/test_regress/t/t_flag_make_cmake_python.pl

Please have this test (or some python test) that writes a VCD have the perl
harness check that the VCD is properly formed:

vcd_identical("$Self->{obj_dir}/simx.vcd", $Self->{golden_filename});

Then run the test with "--gold" once to make the golden file.

 +++ b/test_regress/t/t_flag_make_cmake_python.cmake

Add copyright to top of all new files please.

We have a new feature in #� to do protection of modules. I think
python should work along with that flag as it'll make python more valuable,
but if you want to get this in then work with Todd on getting them to play
together, that seems ok.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-06T14:33:54Z

Note examples have been renamed, see last msg in #�.

Original Redmine Comment
Author Name: Patrick Stewart
Original Date: 2019-10-07T17:41:28Z

I've just pushed a new version to github that should address the above issues. I've stripped out the unrelated formatting and spelling changes, perhaps you/Maarten can deal with pulling those separately.

My preference would be to merge PR #3 over at github that includes python + cmake, then Maarten can do his pydpi patch separately on top of master, because Maarten has suggested over on Github that pydpi isn't ready yet, and it's a significant amount of extra code.

Alternatively, we could just merge cmake instead, and Maarten could merge the fixes I've made into his branch and submit his pydpi branch when it's ready.

Your comment RE incorrect flags and python is exactly why I prefer to outsource that can of worms to pybind11's CMake module.

Maarten: The python gmake support was missing a rule to actually build the module. I've added one in so the example works now, perhaps you've made edits to verilated.mk instead of verilated.mk.inc at some point, or something's gone missing in a git merge?

Original Redmine Comment
Author Name: Maarten De Braekeleer
Original Date: 2019-10-07T23:03:43Z

Patrick Stewart wrote:

Maarten: The python gmake support was missing a rule to actually build the module.
I've added one in so the example works now, perhaps you've made edits to verilated.mk
instead of verilated.mk.inc at some point, or something's gone missing in a git merge?

In src/V3EmitMk.cpp, I change the default target to PREFIX__PYTHON when python support is enabled.
Has this broken somehow?

Wilson Snyder wrote:

I'm getting confused by all the layers. Do you propose

1. cmake goes to master, 2. everything in pydpi?

Or 1. cmake, 2. python no pydpi, 3. pydpi?

If you want to add pydpi separately, file a separate bug and we'll get
through the python support first, then deal with that. Otherwise a bit
more to review at once, but can do it that way too.

We have a new feature in #� to do protection of modules. I think
python should work along with that flag as it'll make python more valuable,
but if you want to get this in then work with Todd on getting them to play
together, that seems ok.

As Patrick Stewart proposed, let's first finish cmake and basic python support.
pydpi support can be squared away later in another issue.
We'll handle the DPI side of #� in there (if any).

Generate a Python module for the design by creating a wrapper using
pybind11 (https://github.com/pybind/pybind11). Python support is only
available for C++ output, not using SystemC. It is recommended to use CMake
to build Python modules BECAUSE WHY?

Based on my experience with Verilog-Perl, expect that the includes & ldflags
reported by python will be incorrect on a lot of systems and will require tweaking
especially on MacOS. I don't think you need to do anything, just a heads-up.

I agree with with Patrick Stewarts view on this.
We cannot be blamed for broken development environments.
Anyway, a way to fix this would be to write a python script that will generate these flags.

First, this is cool. Once we get this merged, I'd love to see a
conference presentation on using this (standalone, as opposed to burried
under Cocotb, which is also of course presentation worthy).

Nice.

I just want to comment that performance wise, it would be interesting to check the difference in performance (if any) between cpython and pypy because the GIL of cpython will trash multithreading. (This is more applicable to pydpi)

+++ b/include/verilated.h

+#ifdef VL_PYTHON
+#include <cstdarg>
+#ifdef VL_PRINTF
+//#warning "VL_PRINTF was already defined. Undefining..."
+#undef VL_PRINTF
+#endif
+#ifdef VL_VPRINTF
+//#warning "VL_VPRINTF was already defined. Undefining..."
+#undef VL_VPRINTF
+#endif

Why do you need to undef VL_PRINTF?

I added these #undef's because otherwise the on_print callback of a Verilated callback would never fire. But maybe it's better to remove the #undef so users can override this behavior.

+++ b/include/verilated.h
-    static void commandArgsAdd(int argc, const char** argv);

Why being removed?

I did not find a Verilated::commandArgsAdd implementation (same with Verilated::commandArgsAddGuts).
There is a VerilatedImp::commandArgsAdd implementation, but Verilated does not inherit from VerilatedImp (same for commandArgsAddGuts).

+++ b/src/V3AstNodes.h
-    string      m_rtnType;              // void, bool, or other return type
+    string      m_rtnTypeString;        // String representation of return type
+    AstBasicDTypeKwd m_rtnType;         // Return type
+    bool        m_context:1;            // Context function

Ok, I guess having this is reasonable and cleaner than how it was
previously. Please make a separate patch off master to add this & related
changes in for this.

This should be in a separate commit in my (now rebased) pydpi branch.

+++ b/src/V3EmitMk.cpp
+#include <memory>

What needs memory?

Nice philosophical question. What needs memory?
640k should be enough. Right?

Anyway,
V3EmitMk uses a vl_unique_ptr for a V3OutCFile.
vl_unique_ptr is #defined in verilatedos.h (instead of typedef'ed) but memory.h is not included in there.
The better solution would be to include in verilatedos.h.

This C file is only generated when using the gmake make system because using cmake, this file is generated using cmake (in include/verialted_py.cmake)
Maybe, the generation of this file should be moved from src/V3EmitMk.cpp to src/V3EmitPy.cpp.

+++ b/src/V3EmitMk.cpp
+            of.puts("default: "+v3Global.opt.prefix()+"__PYTHON\n");

All caps seem ugly unless that's a standard.

I simply copied the __ALL suffix from the lines below it.

+++ b/src/V3EmitPy.cpp

Also too much for me to review at the moment.

There's a lot of code here, please see if there's any overlap with the
normal C output that could be common functions.

Let's postpone pydpi until after merging of cmake and python.

Basically, the big changes in the pydpi branch is converting c types to python types and vice versa. I use information from the ast to convert to the correct types.

@@ -706,11 +708,11 @@ void V3OutFormatter::puts(const char *strg) {
-            if (cp[1]=='\0') {
+            if (cp[1]=='\0' || cp[1]=='\n') {
                 m_prependIndent = true;  // Add the indent later, may be a indentInc/indentDec called between now and th$

Need to think about this, what did it fix? If you have any other
indent/similar changes that are outside python support, let's get them in
first & separately.

If I remember correctly, it was to fix the output of the VPREFIX_py.h file.
I think it had something to do with indentInc and indentDec

Original Redmine Comment
Author Name: Patrick Stewart
Original Date: 2019-10-08T01:52:31Z

In src/V3EmitMk.cpp, I change the default target to PREFIX__PYTHON when python support is enabled.
Has this broken somehow?

The PREFIX__PYTHON target was set as default, but it was empty. The cpp files were all added including the generated one, but there was no rule to link them together into a module either in verilated.mk.inc or V3EmitMk.cpp as far as I could see. I added a rule in verilated.mk.inc to get it done.

Original Redmine Comment
Author Name: Patrick Stewart
Original Date: 2019-10-18T12:58:13Z

I've updated the pull request on top of master now that CMake is merged. I think I've addressed the outstanding issues from here and github. I've also lifted the requirement to set PYBIND11_ROOT when it's installed.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-19T12:23:25Z

Review comments:

+++ b/bin/verilator
+=item --python
+
+This generates a file wraps the toplevel module into a Python module using
+pybind11 (https://github.com/pybind/pybind11). Python support is only available
+for C++ output, not SystemC. The CMake build system is preferred if you
+want to combine multiple verilator modules or add your own C++ code to the
+python module.

s/verilator/Verilator/
s/CMake build system/CMake build system (versus GNUmake)/

The front of the document talks about converting to C++/SystemC. Perhaps should
be C++/SystemC/Python?

+When generating a Verilated Python module, you should configure
+the target correct. This includes: creating a C++ source that will define
+the Python module using pybind11, using the MODULE library target type,
+include the python include directories, link to the python library,
+include the pybind11 include directories, set the suffix of the python
+module such that python will recognize it, and many more.

This needs a bit of rewording, as doesn't read well and not sure if you're
saying these are things that need to be done (need more detail if so) or
not. I think you're saying if you don't use CMake these are some things
that need to be done - if so perhaps better moved to the end of the
section.

+++ b/examples/cmake_python/sim_main.py
+import Vtop

Perhaps

# Import the package that Verilator created for the Verilog module "top"
import Vtop


+++ b/include/verilated.h
+#ifdef VL_PYTHON
+...
+#ifndef VL_PRINTF

Add spaces to show ifdef levels (ignoring the Guard):

+#ifdef VL_PYTHON
+...
+# ifndef VL_PRINTF


+    struct VerilatedVoidCb_list {  ///< Struct holding a list of Flush callback functions
+        VerilatedVoidCb cb;                 ///< Flush callback function
+        struct VerilatedVoidCb_list *next;  ///< Pointer to next flush callback function
+    };
+    static VerilatedVoidCb_list* s_flushCb_list;  ///< List of Flush callback functions

Fix caps and "p" suffix:

+    struct VerilatedVoidCbList {  ///< Struct holding a list of Flush callback functions
+        VerilatedVoidCb cb;                 ///< Flush callback function
+        struct VerilatedVoidCbList* nextp;  ///< Pointer to next flush callback function
+    };
+    static VerilatedVoidCbList* s_flushCb_listp;  ///< List of Flush callback functions

Also this seems separatable, please make a separate patch for this and I'll
push it. (Can just post a patch here - don't need new bug.)

+++ b/include/verilated.mk.in
+ifeq ($(VM_PYTHON),1)
+  CPPFLAGS += $(shell python3-config --includes) -fPIC -DVL_PYTHON -DVL_USER_FINISH -DVL_USER_STOP -DVL_USER_FATAL
+  LDFLAGS  += $(shell python3-config --ldflags) --shared
+  PYTHON_EXT = $(shell python3-config --extension-suffix)
+endif

Use $(PYTHON3_CONFIG) and default-define it as python3-config.

+  $(VM_PREFIX)__PYTHON: $(VM_PREFIX)$(PYTHON_EXT)

Let's rename __Python. I'd rename my __ALL but too late.

+file(WRITE "${O}" "// Verilated -*- C++ -*-\n")
+file(APPEND "${O}" "// DESCR" "IPTION: Generated global Python module\n")
+file(APPEND "${O}" "\n")
+...

I don't see why this is made in the cmake file. I would have expected
Verilator creates it, then gnumake can also benefit.

+++ b/src/V3EmitPy.cpp
+        ofp->puts("static inline auto declare_" + v3Global.opt.prefix() + "(pybind11::module& m) {\n");

I presume pybind requires C++11? We need to document that requirement.

+++ b/include/verilated_py.cpp
+void write_port(vluint32_t* words, size_t width, py::int_ v, bool is_signed) {
+    if (!(i == 0 || (is_signed && i == -1))) {
+        PyErr_SetString(PyExc_OverflowError, "Integer overflow when assigning to wide port");
+        throw py::error_already_set();

if (VL_UNLIKELY(...))

Please check all other if statements for appropriate places for LIKELY/UNLIKELY.

+class VLCallback {

VlCallback

+            throw VerilatedException("Second verilog $finish", s_pymodule);

Does pybind11 expect execption saftey?

+class PyVLCallback : public VLCallback {

PyVlCallback

+    vl_class_mutex
+        .def(py::init<>())
+        .def("lock", &VerilatedMutex::lock,
+            "Try to acquire the lock by spinning.\n"
+            "If the wait is short, avoids a trap to the OS plus OS scheduler overhead.")
+        .def("unlock", &VerilatedMutex::unlock,
+            "Release/unlock mutex")
+        .def("try_lock", &VerilatedMutex::try_lock,
+            "Try to acquire mutex.  Returns true on success, and false on failure.");

Why does python need access to the lock primitives? This seems something
users should not know/will get wrong, and likely to make things slow.

+    vl_class_verilated.def_property_static("vpi_error_fatal",
+        [](py::object) {return Verilated::fatalOnVpiError();},
+        [](py::object, bool v) { Verilated::fatalOnVpiError(v);});
+    attr_doc(vl_class_verilated, "assertions", "Enable/disable vpi fatal");

Doc is a cut and paste error, check others too. Perhaps make a function that declares the property
and also passes documentation so we can avoid this class of error?

Can we have a process to extract this documentation and post it somewhere or make a PDF/HTML?
(Versus duplicating it in bin/verilator which seems evil.)

+++ b/include/verilated_py.h
+/// \brief Verilator: Common include for Verilator python wrappers

s/python/Python/

+#if VM_TRACE_VCD
+#include "verilated_vcd_c.h"
+#endif

Space to show ifdef levels.

+#include "verilatedos.h"

Should always be included first.

+int vl_vprintf(const char* fmt, va_list args);

fmtp. Likewise other places need "p" suffix.

+        if (s) { /* Sign extend */ \
+            struct { decltype(r) v : _VL_PY_ABS(msb-lsb); } sxt { r }; \
+            return sxt.v; \
+        } \

This assumes a platform-specific bit packing endinness.
perhaps "return VL_SIGN_Q(_VL_PY_ABS(msb-lsb), r)"

+#define VL_PY_INPORT(n, p, msb, lsb, s) .def_property(#p, _VL_PY_PORT_READ(n, p, msb, lsb, s), _VL_PY_PORT_WRITE(n,$
+#define VL_PY_OUTPORT(n, p, msb, lsb, s) .def_property_readonly(#p, _VL_PY_PORT_READ(n, p, msb, lsb, s))
+#define VL_PY_INOUTPORT(n, p, msb, lsb, s) .def_property(#p, _VL_PY_PORT_READ(n, p, msb, lsb, s), _VL_PY_PORT_WRITE$

Use VL_PY_IN/OUT/INOUT to match VL_IN/OUT/INOUT. Break up the long lines.

I need to study this file a bit more too.

+++ b/src/Verilator.cpp
-    if (!v3Global.opt.lintOnly()
-        && !v3Global.opt.xmlOnly()

Shouldn't need to have changed these lines.

+    if (v3Global.opt.python()) {
+        V3EmitPy::emitpy();
+    }

I think --lint-only --python should do linting only.

     // Validate settings (aka Boost.Program_options)
     v3Global.opt.notify();
+    if (v3Global.opt.python()) {
+        if (v3Global.opt.systemC()) {
+            v3fatal("SystemC does not support python\n");
+        }
+    }

Move this into notify().

+++ b/test_regress/t/t_python_cmake.cmake
+verilator_add_python_module(example MAIN DOC "An Python module")
+verilate(example PREFIX Vadd THREADS 4 PYTHON SOURCES "${CMAKE_CURRENT_LIST_DIR}/t_python_cmake.v" VERILATOR_ARGS -$
+...

I suspect we'll get more python tests. Perhaps driver.pl should build
the cmake file for python?

+++ b/test_regress/t/t_python_cmake.pl
+if ($Self->have_pybind11 && $Self->have_cmake) {
+    run(logfile => "$Self->{obj_dir}/vlt_python.log",
+        cmd     => [ "python3 $Self->{t_dir}/t_python_cmake.py $Self->{obj_dir}" ]);
+}
+
+vcd_identical("$Self->{obj_dir}/simx.vcd", $Self->{golden_filename});

The vcd check needs to be inside the if().

If the if() fails, print a skip() message.

+++ b/test_regress/t/t_verilated_all.pl
+                         ($Self->have_pybind11 ? "--python "  : ""),

s/ :/ :/