Restore Verilator compile/simulation #2585
Conversation
| tb_top_verilator $(TBSRC_VERI) \ | ||
| -f $(CV_CORE_MANIFEST) \ | ||
| $(CV_CORE_PKG)/bhv/$(CV_CORE_LC)_core_log.sv \ | ||
| $(TBSRC_CORE)/tb_top_verilator.cpp --Mdir $(VERI_OBJ_DIR) \ | ||
| -CFLAGS "-std=gnu++11 $(VERI_CFLAGS)" \ |
There was a problem hiding this comment.
I suppose here "-std=gnu++11" was removed for it is no necessary on Verilator supported (or recent) versions.
I imagine this modification should also be done to the same targets for core-v-verif/cv32e40s/sim/core/Makefile:468 and core-v-verif/cv32e40x/sim/core/Makefile
There was a problem hiding this comment.
This was done as per the discussion of Verilator Issue #4898 which recommends removing -std=c++11.
You are right that we should do the same for cv32e40s and cv32e40x - those should be done on their respective cv32e40[s|x]/dev branches.
| @@ -15,7 +15,7 @@ | |||
| // | |||
| // SPDX-License-Identifier: Apache-2.0 WITH SHL-0.51 | |||
|
|
|||
| module cv32e40p_tb_wrapper | |||
| module cv32e40p_core_tb_wrapper | |||
There was a problem hiding this comment.
Was there a particular reason for the renaming? I agree that adding _core looks better, but now the name of the file should also follow it :)
There was a problem hiding this comment.
Good catch! You are right, the filename should have been updated to match the module name. That also means updating the file-list in the Makefile. I have pushed in these changes on this pull-request.
There was a problem hiding this comment.
The modifications do allow Verilator to work again, solving #2585 - I tested it locally on Verilator 5.033.
I advise to change the filename of the cv32e40p/tb/core/cv32e40p_tb_wrapper.sv to cv32e40p/tb/core/cv32e40p_core_tb_wrapper.sv, and to check if the -std=gnu++11 CFLAG can be removed on Verilator targers for other cores too
Resolves #2584, and cleans up DSim compiles as well.