Working Xcelium + Indago support with single command commpilation plus a basic rerun script for Xcelium #866

Hardolaf · 2022-11-01T22:26:01Z

This work started from the work that @rodrigomelo9 presented in his PR: #728.

Incisive and irun were replaced by Xcelium and xrun.

… warnings

…ath not supported)

… Xcelium

Added to ignore a too-many-branches complain of pylint due to the additions.

…s a basic rerun script.

GlenNicholls · 2022-11-02T18:02:30Z

vunit/sim_if/xcelium.py

+        self._prefix = prefix
+        self._libraries = []
+        self._log_level = log_level
+        if cdslib is None:


Why check for None instead of doing:

if cdslib: ... else: ...

Shorter if it fits reasonably in VUnit's line length limit (if there is one):

x = a if cdslib else b

EDIT:

And if you have a single default if not set, you could do something like:

def func(arg: Optional[str]): arg = arg or str(Path(arg).resolve())

This is actually as per https://peps.python.org/pep-0008/#programming-recommendations, but it could be that an unset cdslib should have a default of "" which would then be tested as you show.

Well yeah, but only if you need to know that the value is actually None. However, all that is being checked here is if the value is set, otherwise it's being set to the default.

In this case, it is written as an anti-pattern. My comment implicitly (sorry) recommends avoiding this pattern by using the more logical pattern of handling None and other "empty" objects:

if not value: do this else: do that

Why would you check for "false" first if you can just do the following which is less confusing to read (more important for complicated conditionals):

if value: do that else: do this

None is falsy meaning you can safely use if [str, None] and it will never be true for None. The other advantage of not checking None here is that it will also catch empty objects like "", [], (), {}, etc. and fallback to the default behavior. As it is written, those objects will require the user to read through a code traceback when it inevitably fails because the user used the wrong empty type. I think it's important here because there's no type hint, so I'm not actually sure what cdslib is supposed to be (from reading the interface, not the code).

Regarding the PEPs warning:

Also, beware of writing if x when you really mean if x is not None – e.g. when testing whether a variable or argument that defaults to None was set to some other value. The other value might have a type (such as a container) that could be false in a boolean context!

It doesn't recommend one way or another. However, I use type-hints for this reason so the user knows what is expected and what isn't. In that case, it is safe to make assumptions about how the data can be handled as the interfaces requirements are explicit.

cdslib defaults to None, this is the correct check. In terms of type-hinting, we didn't feel it was our place to universally add type-hinting throughout all of VUnit as much as it pained us to not do so.

I should also clarify that "" is a valid input for this field. If the user does not specify, or does not wish to specify the cds.lib file, then Xcelium will infer the location of the file and handle it automatically.

In terms of type-hinting, we didn't feel it was our place to universally add type-hinting throughout all of VUnit as much as it pained us to not do so.

I totally understand that.

"", at least to me, is not obvious that it's supposed to be CWD unless I knew the implementation. Yes, Path("").resolve() resolves to the CWD, but this to me isn't clear. I would think that if a user wanted CWD, they would pass ".". If I were reading the code and "" was used, I would read it as the user letting the code choose the default. My preference is to be explicit, not implicit and in this case I think treating "" as a valid path is implicit.

EDIT: If other parts of VUnit do this, then stick with that as I'm not sure to be honest. I just wanted to point it out as if not ... elif ... can usually be refactored slightly to be more clear.

GlenNicholls · 2022-11-02T18:03:42Z

vunit/sim_if/xcelium.py

+        """
+        cds_root_virtuoso = self.find_cds_root_virtuoso()
+
+        if cds_root_virtuoso is None:


I've never understood driving TCL interfaces with Python strings. Stuff like this is way clearer when using Jinja templates as the flow can be followed in a single file with all the for, if/else, etc. constructs instead of following python strings between functions/methods

For multi-line strings, I like using inspect.cleandoc instead to avoid \'s and also to keep the string inline with the code

Is Jinja already a VUnit dependency? If so, I agree, otherwise it's not quite so clear if that code warrants an extra dependency.
I didn't know about inspect.cleandoc, interesting. I often see textwrap.dedent used (assuming the tabs --> spaces transformation is not needed).

If nothing else, this should use f-strings.
I think I was using Python 3.2 when I originally wrote this which apparently didn't even support {} with auto-numbering.

I'm not sure, it's been a while since I've forked and modified VUnit. textwrap probably makes more sense, I know cleandoc because I've needed some other stuff it can do that dedent can't.

Ideally, I'd just convert this to format strings. But I don't see a need to change it unless people are very unhappy with it.

GlenNicholls · 2022-11-02T18:06:03Z

vunit/sim_if/xcelium.py

+        """
+        Compiles all source files and prints status information
+        """
+        try:


Is the output consistent with the rest of VUnit's reporting?

If I had to guess, the answer would be no.

GlenNicholls · 2022-11-02T18:07:05Z

vunit/sim_if/xcelium.py

+        for source_file in source_files:
+            unique_args = []
+            if source_file.is_vhdl:
+                unique_args += ["%s" % self._vhdl_std_opt(source_file.get_vhdl_standard())]


f-string would be clearer

GlenNicholls · 2022-11-02T18:07:39Z

vunit/sim_if/xcelium.py

+
+            args += [source_file.name]
+            if len(unique_args) > 0:
+                # args += ["-filemap %s" % source_file.name]


It'd be nice to remove commented code if it's not needed anymore. If so, else should be removed entirely

We left this commented code in because we didn't have time to setup a test for it but we'd ideally like to support per-source file arguments as is supported by Xcelium.

I see. In that case, I'd still remove it, but open a new issue with some of this code and where it should go unless you plan to add support.

GlenNicholls

Whoops, didn't mean to start review.

GlenNicholls · 2022-11-02T18:09:07Z

vunit/sim_if/xcelium.py

+        cmd = str(Path(self._prefix) / "xrun")
+        args = []
+
+        args += ["-compile"]


It would be clearer to just do

args = [ "foo", "bar", ]

GlenNicholls · 2022-11-02T18:10:46Z

vunit/sim_if/xcelium.py

+        Returns command to compile a VHDL file
+        """
+        cmd = str(Path(self._prefix) / "xrun")
+        args = []


It would be clearer to do:

args = [ "foo", "bar", ]

GlenNicholls · 2022-11-02T18:13:35Z

vunit/ui/__init__.py

@@ -795,6 +818,11 @@ def _main_run(self, post_run):
        Main with running tests
        """
        simulator_if = self._create_simulator_if()
+        try:
+            simulator_if.set_global_compile_options(self._global_compile_options)


Why is every python exception caught and not the ones you care about? This could mask potentially important issues in the code that would make debugging difficult

Also, it could be written more succinctly using with suppress(Exception): (and from contextlib import suppress)

Do you know why this is catching all errors though? My concern is things like KeyError somewhere below that should be raised and might cause confusing errors later. My opinion is that this should only catch what it cares about to avoid masking VUnit bugs that affect the user.

Every once in a while I need this pattern, but very very rarely. Usually only near the top level where I'm re-raising the exception because I knew something bad happened that I didn't expect and want to provide extra context (e.g. a log file)

I believe this was added because we didn't care if it failed which is probably not the behavior we'd actually want in production quality code. I'll have to dig in and see what possible failures actually exist and see if we even care to catch any of them.

tools/xcelium_verilog_fixup.py

GlenNicholls · 2022-11-02T18:20:14Z

vunit/sim_if/incisive.py

-            default=None,
-            help="The hdl.var file to use. If not given, VUnit does not use a hdl.var file.",
-        )
+    # NOTE: Incisive shares the command-line arguments with Xcelium


This comment relates to the code that was removed, this should be in a commit message instead. I personally don't think this comment is needed, but I would clean it up to include add_argument is excluded because X

IMO we could even remove all traces of Incisive, its last release was in 2015, everything after that was called Xcelium.

Would the Incisive from 2015 work with the new Xcelium interface?

I honestly don't know. Our codebase is incompatible with Incisive due to reliance on newer System Verilog features.

GlenNicholls · 2022-11-02T18:21:45Z

vunit/sim_if/xcelium.py

+            ]  # promote to error: "bad natural literal in generic association"
+            args += [
+                '-xmlibdirname "%s"' % (str(Path(self._output_path) / "libraries"))
+            ]  # @TODO: ugly


with f-string, you wouldn't need to type-cast as str

GlenNicholls · 2022-11-02T18:22:39Z

vunit/sim_if/xcelium.py

+        cds = CDSFile.parse(self._cdslib)
+        return cds
+
+    def simulate(  # pylint: disable=too-many-locals, too-many-branches


This whole function is really complex and confusing. I would break things up a bit or find a better way to do all the branching.

GlenNicholls · 2022-11-02T18:24:07Z

vunit/sim_if/xcelium.py

+        args = []
+        for name, value in generics.items():
+            if _generic_needs_quoting(value):
+                args += ['''-gpg "%s.%s => \\"%s\\""''' % (entity_name, name, value)]


First, this seems appropriate for list comprehension. Second, why are there tripple '''?? This file has been using " for most quotes so this was awkward at first to read.

I think this code was originally mine (back when f-strings were too new), and the triple single-quotes make this more readable IMO - it's a string with quotes that contains escaped quotes.
I agree with the use of a list comprehension and would also use f-strings now

That's fair. I would honestly run this file through black with preview features enabled so it reformats strings and see what it comes up with.

GlenNicholls · 2022-11-02T18:25:37Z

vunit/source_file.py

 import logging
 from copy import copy
 import traceback
-from vunit.sim_if.factory import SIMULATOR_FACTORY


Why was this import moved to the funcs/methods? It has the potential to be imported many, many times now which seems pointless to me.

We were having an issue with it for some reason. I have it on my list of things to clean up/fix.

GlenNicholls · 2022-11-02T18:30:44Z

tests/unit/test_xcelium_interface.py

+                "-nowarn DLCVAR",
+                "-v200x -extv200x -inc_v200x_pkg",
+                "-work work",
+                '-cdslib "%s"' % str(Path(self.output_path) / "cds.lib"),


I strongly dislike mixing quote types for no reason. Use f-string or format to clean this up.

I'm not sure, but it could be that the simulator really wants to see doublequotes here.

I mentioned black earlier (I won't reference it more after this), but their docs/impl might have their str opinions about this case. Not sure if the PEPs have a recommendation for this. It doesn't look like VUnit enabled preview features so that's why it's probably not touching strings.

GlenNicholls · 2022-11-02T18:33:53Z

tests/unit/test_xcelium_interface.py

+    Test the Xcelium interface
+    """
+
+    @mock.patch("vunit.sim_if.xcelium.XceliumInterface.find_cds_root_virtuoso")


Similar mocks are used for most test methods. I would decorate all the common ones on the class insted. For any duplicated code, you can set up a static or class method to configure the default mocks or leave unique mocks decorated local to the test methods if it's not shared.

tools/xcelium_verilog_fixup.py

cmarqu · 2022-11-02T19:27:35Z

vunit/sim_if/xcelium.py

+            help="The hdl.var file to use. If not given, VUnit does not use a hdl.var file.",
+        )
+        group.add_argument(
+            "--use_indago",


Suggested change

"--use_indago",

"--use-indago",

Python tools generally don't use underscores in options.

I don't think VUnit does either.

vunit/sim_if/xcelium.py

cmarqu · 2022-11-02T19:48:02Z

vunit/ui/__init__.py

@@ -795,6 +818,11 @@ def _main_run(self, post_run):
        Main with running tests
        """
        simulator_if = self._create_simulator_if()
+        try:
+            simulator_if.set_global_compile_options(self._global_compile_options)


Also, it could be written more succinctly using with suppress(Exception): (and from contextlib import suppress)

GlenNicholls · 2022-11-03T16:29:25Z

Another quick thing I noticed is there is already #728. Does anyone know how this differs?

Hardolaf · 2022-11-04T15:00:45Z

tools/xcelium_verilog_fixup.py

@@ -0,0 +1,60 @@
+# This Source Code Form is subject to the terms of the Mozilla Public


This file should be removed as it is no longer necessary.

Is it needed for older versions of Xcelium?

No. It was added by @rodrigomelo9 in their original PR and was made irrelevant by our inclusion of the patches to the files in this PR.

Hardolaf · 2022-11-04T15:06:26Z

@GlenNicholls As mentioned in the opening of this PR, this is based on #728 which was incomplete. If you look at the commit history for this PR, it has the full history from #728 included.

rodrigomelo9 · 2022-11-04T15:36:40Z

Hi. I started #728, but I'm not a VUnit user, so when it started to be complicated, I left it unfinished...
Happy to see that somebody continues this work and hopefully, it will be part of VUnit finally! Thanks.

GlenNicholls · 2022-11-04T15:53:04Z

@GlenNicholls As mentioned in the opening of this PR, this is based on #728 which was incomplete. If you look at the commit history for this PR, it has the full history from #728 included.

I see, thanks for the clarification!

Hardolaf · 2022-11-04T21:07:51Z

I did a first pass on cleaning up the review per the comments. I've addressed most issues so far locally, but I haven't had time to finish my changes and verify them against xcelium this week. I'll probably get a new commit up sometime next week, time permitted.

LarsAsplund · 2022-11-07T06:45:47Z

tools/xcelium_verilog_fixup.py

@@ -0,0 +1,60 @@
+# This Source Code Form is subject to the terms of the Mozilla Public


Is it needed for older versions of Xcelium?

LarsAsplund · 2022-11-07T07:00:58Z

vunit/sim_if/incisive.py

-            default=None,
-            help="The hdl.var file to use. If not given, VUnit does not use a hdl.var file.",
-        )
+    # NOTE: Incisive shares the command-line arguments with Xcelium


Would the Incisive from 2015 work with the new Xcelium interface?

LarsAsplund · 2022-11-07T07:28:48Z

vunit/ui/__init__.py

@@ -489,6 +492,26 @@ def add_compile_option(self, name: str, value: str, allow_empty: Optional[bool]
        for source_file in check_not_empty(source_files, allow_empty, "No source files found"):
            source_file.add_compile_option(name, value)

+
+    def set_global_compile_options(self, name: str, value: str, allow_empty: Optional[bool] = False):


Is this a way to add compile options to future added source files? Is that necessary for Xcelium support or a separate feature?

This was needed to achieve acceptable performance for Xcelium via single command compilation (We saw compilation times go from ~2m to ~11s on our NFS share). The other option was adding all compilation options into a hash map and then de-duping them, but in early testing that proved to be very error prone based on how users might decide to compose command line options in Python.

LarsAsplund · 2022-11-07T07:38:18Z

vunit/verilog/vunit_pkg.sv

@@ -79,7 +79,11 @@ class test_runner;
      end

      if (index == -1) begin
-         $error("Internal error: Cannot find 'enabled_test_cases' key");


Generally speaking we have controlled the behavior on errors by configuring the simulator. The reason is that we want the error to behave the same way regardless if it is caused by user code or VUnit features. Is that not possible with Xcelium?

I believe we added this because it was the easiest way to ensure consistent behavior between Questa/Modelsim and Xcelium. We can look into configuring the max errors before stopping in Xcelium but I haven't done it before.

JFMarten · 2023-08-22T12:00:50Z

I did a first pass on cleaning up the review per the comments. I've addressed most issues so far locally, but I haven't had time to finish my changes and verify them against xcelium this week. I'll probably get a new commit up sometime next week, time permitted.

Any updates regarding this MR? I would like to use VUnit with Xcelium in an upcoming project. If you need any assistance, let me know. Maybe i can somehow contribute.

bryankerr1986 · 2023-08-31T23:12:10Z

FYI I decided to try this branch with Xcelium 2303 using the Uart example run.py - Failed. Seemed like OSVVM wasn't even compiling correctly.
Edit: "After making a few modifications to this branch and rebasing to the latest release, I was able to get simulations running with the latest 23.03 Xcelium"

Hardolaf · 2023-09-29T17:55:50Z

@JFMarten Due to leaving the position where I worked on this, I won't be able to continue the work on this for contractual reasons. I wasn't able to get this into a good linted state before leaving the job. The branch does work for System Verilog and Verilog. For VHDL, we didn't have anything other than toy examples to try and those appeared to work.

@bryankerr1986 I believe that is likely an OSVVM + Cadence Xcelium issue not an issue with this branch if I'm remembering the communications with Cadence correctly. There were a lot of OSVVM issues in Cadence and they got fixed after this branch was created so we never got the fixes in the git submodule into this branch.

For anyone who is capable of picking this up, please feel free to do so.

bryankerr1986 · 2023-09-29T18:12:08Z

@Hardolaf

Ok, well I forked your repo and rebased to the latest VUnit. I also added an option to pass compile arguments. Now I have a pretty large vhdl sim working.

One thing I noticed is that your compiling the source files a little differently than incisive was doing this right? I noticed run.py --compile and run.py -m we're not working for me. Does this sound familiar?

Hardolaf · 2023-09-29T18:29:56Z

@bryankerr1986 We ran into a lot of issues around NFSv4 and file timestamps with Xcelium when we tried doing multi-library compiles. We also found that the per-file compilation was fundamentally broken both in terms of speed and in terms of maintaining a consistent database on NFSv4 drives (and we saw the same issue on a GlusterFS filesystem). We also tried several parallel build and simulation trials but didn't get good results using a common compilation directory with per-sim elaboration keys.

And yes, I did break a good number of things from what was being done for Incisive. Overall, VUnit is not designed to handle 3-step compilation or efficient compilation particularly well. And we didn't really fully test everything. We got it to a working state for us and pushed that. Additional changes were made but I never got approval to push them. But those are mostly around automation in Indago not around the compilation and simulation flow without a debugger attached.

Overall, we were in the process of moving away from VUnit as it was too inflexible for the number of simulations we were running in CI/CD and it was not scaling well.

rodrigomelo9 and others added 13 commits August 31, 2021 10:27

vunit/sim_if/xcelium.py: added as a modified copy of incisive.py

862d5fc

Incisive and irun were replaced by Xcelium and xrun.

xcelium: replaced nclibdirname (deprecated) by xmlibdirname, to avoid…

f85ba24

… warnings

xcelium: replaced ncerror by xmerror

9ef0feb

Fixed acceptance testing issue with Incisive and Xcelium (spaces in p…

1717700

…ath not supported)

Fixed an issue with the shared command-line arguments of Incisive and…

d204405

… Xcelium

Fix compile issues under vunit/vhdl/data_types/src

fbd3c7c

Added options into xcelium.py taken from VUnit#684

cfc070a

Added to ignore a too-many-branches complain of pylint due to the additions.

tools: add xcelium_verilog_fixup.py

12301ac

Modified to find Xcelium before Incisive

e7fc206

Added test for the Xcelium interface

36d1d17

Merge branch 'master' of github.com:VUnit/vunit into add/xcelium-support

aa7ae54

Fix string type error by decoding bytes returned by subprocess

9d3f446

Working Xcelium + Indago support with single command commpilation plu…

cd70d5d

…s a basic rerun script.

GlenNicholls reviewed Nov 2, 2022

View reviewed changes

tools/xcelium_verilog_fixup.py Show resolved Hide resolved

GlenNicholls reviewed Nov 2, 2022

View reviewed changes

cmarqu reviewed Nov 2, 2022

View reviewed changes

Hardolaf commented Nov 4, 2022

View reviewed changes

LarsAsplund reviewed Nov 7, 2022

View reviewed changes

		@@ -0,0 +1,60 @@
		# This Source Code Form is subject to the terms of the Mozilla Public

Working Xcelium + Indago support with single command commpilation plus a basic rerun script for Xcelium #866

Are you sure you want to change the base?

Working Xcelium + Indago support with single command commpilation plus a basic rerun script for Xcelium #866

Conversation

Hardolaf commented Nov 1, 2022

GlenNicholls Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlenNicholls Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlenNicholls Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlenNicholls Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlenNicholls left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlenNicholls Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlenNicholls Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlenNicholls Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlenNicholls commented Nov 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hardolaf commented Nov 4, 2022

rodrigomelo9 commented Nov 4, 2022

GlenNicholls commented Nov 4, 2022

Hardolaf commented Nov 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hardolaf Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JFMarten commented Aug 22, 2023

bryankerr1986 commented Aug 31, 2023 • edited Loading

Hardolaf commented Sep 29, 2023

bryankerr1986 commented Sep 29, 2023

Hardolaf commented Sep 29, 2023

GlenNicholls Nov 2, 2022 •

edited

Loading

GlenNicholls Nov 2, 2022 •

edited

Loading

GlenNicholls Nov 4, 2022 •

edited

Loading

GlenNicholls Nov 2, 2022 •

edited

Loading

GlenNicholls Nov 2, 2022 •

edited

Loading

GlenNicholls Nov 2, 2022 •

edited

Loading

GlenNicholls Nov 2, 2022 •

edited

Loading

Hardolaf Nov 8, 2022 •

edited

Loading

bryankerr1986 commented Aug 31, 2023 •

edited

Loading