Fix trace tb generation #1680
Fix trace tb generation #1680
Conversation
Define `genclock` in non Verilator block to avoid redefining it.
Without this commit the Verilog testbench produced by `write_vlogtb_trace()` will contain invalid signal paths for const and seq type cells. Only the signal name is available but not the actual path to the signal. In the following example both signals are defined in `wrapper.mod`. ``` UUT.const_test = 2'b00; // `const_test` is not defined in UUT UUT.wrapper.mod.reg_test = 2'b00; ``` The trace is produced by `smtbmc.py` which uses the stm2 format as an input. Information stored inside of Yosys specific comments are used to get the actual path of the signal. These comments are created by `smt2.cc`. For the previous example the comments would differ like in the following. ``` ; yosys-smt2-anyconst tb#0 2 tb.sv:5|wrapper.sv:8|mod.sv:8 const_test ; yosys-smt2-register wrapper.mod.reg_test 2 ``` Store the hierarchy to const and seq cell types in a similar format as for registers. Use the name then for the creation of the Verilog testbench. ``` ; yosys-smt2-anyconst tb#0 2 wrap.mod.const_test const_test ``` The format is kept similar to the original way in order to preserve checks already in place.
nakengelhardt
added
the
discuss
| infostr += " " + cell->attributes.at("\\reg").decode_string(); | ||
| decls.push_back(stringf("; yosys-smt2-%s %s#%d %d %s\n", cell->type.c_str() + 1, get_id(module), idcounter, GetSize(cell->getPort("\\Y")), infostr.c_str())); | ||
| infostr += cell->attributes.at("\\reg").decode_string(); | ||
| decls.push_back(stringf("; yosys-smt2-%s %s#%d %d %s %s\n", cell->type.c_str() + 1, get_id(module), idcounter, GetSize(cell->getPort("\\Y")), get_id(cell->getPort("\\Y").as_wire()), infostr.c_str())); |
There was a problem hiding this comment.
There's value to providing a src attribute in the smt comment when present (this change completely removes that information) and there's no guarantee that cell->getPort("\\Y").as_wire() doesn't return a NULL pointer afaict.
There was a problem hiding this comment.
Thanks for the feedback.
Would you advice to add this info, if available, as an addition? Not sure anymore how this would affect retrieving of the info, I think I tried to align this with other cases.
Or does this mean changes to this comment should not be made?
There was a problem hiding this comment.
I think I'm not too picky about the details, just think this information should be present, and that .as_wire() is no good here for the reasons stated above.
cliffordwolf
removed
the
discuss
|
@towoe are you still interested in finishing this PR? What is blocking it? |
|
@nakengelhardt I think it would make sense to get the hierarchy information in for const signals as well so this can be reused in a similar way as with the other signals. |
|
I'm not familiar with the smt2 format. Is everything in If you have a small testcase that I could play around with to see the output, and the error that it causes, that might help. For the |
This PR provides some improvements for the generation of Verilog test benches.
Please have a look at the commit messages for a detailed description of the changes.
There are still some problems. But it would be nice to get some feedback if the changes make sense.
If I have a solution for #1676 I can update this PR to include the changes. Or this can be done independently.