Support Questa qrun/QIS flow #3306
Conversation
|
If we're OK with the flow in general I'll add the same commands to the runner as well (probably in a separate PR). |
|
@imphil Yeah sure, thanks for you efforts. I am looking into this and testing it now, I will update you once I am done |
|
Needs a newsfragment. |
|
Thanks for your efforts @imphil, It is working well, the info messages also are great and keep the user aware of the changes done. also variables names looks better now. We are OK with this, you can go head. Thanks |
|
Sorry, can we add the python architecture check again to Makefile.questa-qrun, I assumed user will point to only one binary and forgot the case when user has both 32 and 64 binaries in same path, this should not impact anything else. |
b3b508c to
01fb79f
Compare
|
Updated to include the 64b switch as proposed as well as a newsfragment. I'll wait with merging until we have Questa 2023 in CI (currently blocked for unrelated test failures). |
01fb79f to
e0e12dc
Compare
|
OK, we now got Questa 2023.2 in CI, together with Questa 2022.4. All tests pass with 2022.4 using the +acc old flow. Unfortunately, a number of tests fail with the new QIS/Qrun flow: The failures look on first sight like differences in signal visibility due to the QIS flow. @AbdouTharwat Can you reproduce these test failures with this PR? You should get the same failures if you run the tests against Questa 2023.2, as described at https://github.com/cocotb/cocotb/blob/master/CONTRIBUTING.md#running-tests-locally. I can try to piece together steps to reproduce these failures and file an issue with Siemens, but you're probably in a much better position to do so. Let me know if you need further input from my side. |
|
Thanks @imphil, I have reproduced these 4 test failures and filed an issue. We are now looking into this |
|
So what can we do for now to get this in for 1.8? Do we add conditionals for both version and whether the qrun mode is used? We can only really detect if we are using qrun if use the environment variable, but perhaps that's fine here since this is a part of the regression where we are only using makefiles (for now). |
|
I'd just leave this out of 1.8, we're not ready yet to move users over to the new flow without regressions. Users can still use the older flow until we understand the regressions better. |
|
Hello @imphil, after having a look on the failures you mentioned I have some observations: in test_iteration_verilog --> testcase: recursive_discovery
in test_iteration_mixedlang --> testcase: recursive_discovery
in test_iteration_vhdl (FLI) --> testcase: recursive_discovery
in test_array --> testcase: recursive_discovery
We are investigating other issues of:
|
|
Why are we even trying to discover always, initial, and function blocks? |
e0e12dc to
c9d729c
Compare
|
@AbdouTharwat Thanks for looking into the fails, much appreciated! I pushed a number of commits to address some of the issues that you found plus a couple more, most notably:
TODOs:
|
History, I guess :-) I don't think we have much use for these blocks ultimately. |
44809a3 to
9f06918
Compare
|
I looked at the runs and that's what I'm getting with the QIS/Qrun flow with Questa 2023.2: VHDL-VHPI
VHDL-FLI
Verilog-VPI
|
|
Hi @imphil, Regarding the VPI tests, there are some observations: test_array
50.00ns INFO cocotb.test ------Found HierarchyObject(array_module.asc_gen[20].#ALWAYS#182) (GPI_MODULE) Reviewing analog_module.sv we find:
In conclusion it appears that QIS shows objects that were not shown by old flow but are correct as shown above test_cocotb_arraythe test was run with LOG_LEVEL DEBUG and for the 3 mentioned tests test_in_vect_packed_packed_unpacked, test_in_arr_packed_unpacked, and test_in_2d_arr_unpacked it appears that old flow failed to find handle for those while new QIS flow succeeded. I am attaching the debug log for the 3 tests both in QIS flow and +acc flow so you can have a look Reviewing the test_cocotb_array.py file we find 3 comments at lines 85, 125, 154 stating that Questa is unable to access elements of a logic array if the last dimension is unpacked (gh-2605) which means it is an open issue and it appears to be solved with new QIS flow In conclusion it appears that these tests were failing earlier but works well with QIS flow. So they should be modified now that QIS flow is able to access those elements, do you agree? Finally regarding FLI and VHPI failures, we already opened issues for those and are being investigated, so we are on track. Thanks, |
|
@AbdouTharwat I'm going to fix #3353 then this back and forth should be moot. |
|
What happened to this PR? I'd quite like to use this flow and it looks as though it never got merged |
|
@sirgajelot Looks like it was blocked by another PR that was never merged. That other PR was blocked because it requires restructuring code in the GPI which is a PITA and is not my current focus since I'm working on cocotb 2.0, which is mostly Python API changes. |
|
Am I right that it works ok with purely Verilog / SystemVerilog designs? I found that it produced identical results to my Modelsim runs when using this branch |
|
If it works, feel free to use it; everything is BSD licensed. There is nothing conceptual wrong with the change, so it will eventually make it in. |
9f06918 to
19d4e7a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3306 +/- ##
===========================================
- Coverage 77.31% 67.17% -10.14%
===========================================
Files 53 53
Lines 8286 8262 -24
Branches 2026 2007 -19
===========================================
- Hits 6406 5550 -856
- Misses 1410 2286 +876
+ Partials 470 426 -44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
19d4e7a to
89ee6d9
Compare
89ee6d9 to
23543a9
Compare
23543a9 to
0c3f1f0
Compare
ea4a390 to
070ecc4
Compare
questa_fli is only active if we're in a VHDL design; add the missing condition. Also refactor the code slightly for consistency
Updated Makefile for Questa simulator to use the new flow QIS and qrun instead of +acc and vlog/vsim respectively. Also using Visualizer instead of the classic GUI for Live simulation and adding option for Visualizer post simulation mode By Default Questa runs in batch mode, to enable Live SIM mode set GUI=livesim, to enable Post SIM mode set GUI=postsim Added vis cmd for Visualizer beside vsim cmd for Post SIM mode, and added same checks for the cmd as vsim Changed way of invoking Questa from running .DO file into directly invoking Questa from bash/shell terminal VSIM_ARGS and other variables kept untouched for backward compatibility, also SCRIPT_FILE is kept for user custom DO file Added more variables for new design.bin and qwave.db file names for user custom preferences Fixes cocotb#2852
|
VHDL+VHPI and Verilog VPI are clean now. We are left with failures in VHDL+FLI at this point. @AbdouTharwat, you wrote:
We are now down to the VHDL/FLI failures that we already saw in the initial run of this PR (failures at Can you check what came out of the issues that you reported and let us know the issue number so we have a number to reference? |
| The QIS/qrun flow is chosen automatically if Questa 2023.1 or newer is detected. | ||
| Users can explicitly use the QIS/Qrun flow with :make:var:`SIM=questa-qisqrun`. |
There was a problem hiding this comment.
Will this cause an issue where the user starts with an old Questa or selected compat and the arguments and all that not working when it magically decides to use the new flow?
| GPI_EXTRA := $(shell $(PYTHON_BIN) -m cocotb_tools.config --lib-name-path $(VHDL_GPI_INTERFACE) questa):cocotb$(VHDL_GPI_INTERFACE)_entry_point | ||
| endif | ||
| # Determine the version of Questa being used. | ||
| QUESTA_VERSION := $(shell $(CMD) -version | python -c 'import re,s 10000 ys; print(re.sub(r".+vsim (\d+)\.(\d).+", "\\1 \\2", sys.stdin.read()))') |
There was a problem hiding this comment.
| QUESTA_VERSION := $(shell $(CMD) -version | python -c 'import re,sys; print(re.sub(r".+vsim (\d+)\.(\d).+", "\\1 \\2", sys.stdin.read()))') | |
| QUESTA_VERSION := $(shell $(CMD) -version | $(PYTHON_BIN) -c 'import re,sys; print(re.sub(r".+vsim (\d+)\.(\d).+", "\\1 \\2", sys.stdin.read()))') |
| $(SIM_CMD_PREFIX) $(QRUN_CMD) $(RUN_ARGS) -outdir $(SIM_BUILD) \ | ||
| $(foreach LIB, $(VHDL_LIB_ORDER), $(make_lib)) \ | ||
| -makelib $(TOPLEVEL_LIBRARY) $(VERILOG_SOURCES) $(VHDL_SOURCES) $(VLOG_ARGS) $(VCOM_ARGS) -end \ | ||
| $(VOPT_ARGS) $(VSIM_ARGS) $(EXTRA_ARGS) $(call deprecate,PLUSARGS,COCOTB_PLUSARGS) +define+COCOTB_SIM -sv \ |
| @@ -14,18 +14,16 @@ | |||
|
|
|||
| SIM_NAME = cocotb.SIM_NAME.lower() | |||
| SIM_VERSION = cocotb.SIM_VERSION | |||
| LANGUAGE = os.environ["TOPLEVEL_LANG"].lower().strip() | |||
| LANGUAGE = os.getenv("TOPLEVEL_LANG", "verilog").lower().strip() | |||
| VHDL_GPI_INTERFACE = os.getenv("VHDL_GPI_INTERFACE", "") | |||
There was a problem hiding this comment.
Didn't you change the default for getenv in your last PR because we are always exporting it now?
| return 5119 | ||
| else: | ||
| # The QIS/Qrun flow additionally finds | ||
| # Found dec_viterbi_ent.#IMPLICIT# (<class 'cocotb.handle.IntegerObject'>) |
|
Hi @imphil, great news. Actually, we already fixed FLI issues and were looking into VHPI failure. Since you are saying that now VHPI also is clean, then I think we are done. The fix for FLI issues should be available in upcoming Questa 2025.2 which is expected to be out in mid-June 2025. However, I can get you a Questa build with the fix to try before the official release if you wish to validate the fix early Let me know if you wish to have the build and I will have it ready by next week |
|
@AbdouTharwat Yes, please send me a pre-release build of Questa to confirm the fix on FLI. For VHPI, we do see slightly different behavior during discovery as mentioned at #3306 (comment). That's not super problematic, but would ideally be fixed as well. Let me know if you have a issue number for that and I'll add it to the comment to make sure we can follow up on it. |
|
Hi @imphil, I shared a pre-release with you in order to validate FLI fix. Also, regarding the VHPI issue it is tracked in our system by ticket QSIM-84124 |
Support the qrun/QIS flow in Questa in the Makefile flow
Closes #4364.
v2 from #3274:
@AbdouTharwat I kept the original commit attributed to you, since you did the majority of the work. Thank you for it! Can you please have a look and give it a try?
Patches:
TODOs: