Adding Verilator and a basic Verilator unit test

[teqdruid]

• Adds Verilator as an optional test feature.
• Adds basic Verilator unit test. (To check for Verilator changes when we upgrade.)
  • Unfortunately Verilator tends to exit before printing all the errors or warnings, so we're gonna have to have a few error unit tests.
• Adds script to download and compile known good version of Verilator.

Integration tests and workflow to support running them will be a subsequent PR.

[teqdruid]

Do we want to require Verilator or just make it required for some tests? Given that most dialects will be producing Verilog at the end of the day, I think it's reasonable to just require it. We can't test end-to-end if we don't have a simulator, yes?

[lattner]

We should not require verilator, but it is ok to have verilator as an optional tool that is enabled at cmake time. I would recommend making the presence of verilator be a "lit" feature, so we can write specific tests with "REQUIRES: verilator".

[drom]

you could run verilator in GitHub CI only using Docker container: https://hub.docker.com/r/verilator/verilator

jobs:
  sim-verilator:
    runs-on: ubuntu-latest
    container:
      image: verilator/verilator
    steps:
      - name: checkout
        uses: actions/checkout@v1

      - name: sim
        run: |
          verilator --version

[teqdruid]

@lattner Fair enough. AFAICT, lit "REQUIRES:" apply to the whole file. Unless I'm wrong about that we have three options: 1) Keep require verilog on the files where I'm doing it now (and presumably all tests which produce SystemVerilog) and have them not run if verilator isn't installed. 2) Copy the files and have one for Verilator and one for FileCheck. 3) Select on a test-by-test basis. Which should I go with? (I suspect #3.)

[lattner]

Right, we shouldn't run it on all the testcases, that breaks the spirit of unit testing. We should instead run a corpus of stuff against verilators lint rules, distill failures down to small examples, and encode those as tests that we capture in the testsuite.

[teqdruid]

Update: I found that I can override the entrypoint that the verilator image specifies! There are some other complications regarding the different environments which I'll look into.

@drom Running the tests in a container would -- at this point -- require running the entire compilation in said container, which I don't think we want to do. The other workflow is to run the Verilator docker image as a step (very nice for the common use case) but their Docker image is set up to run Verilator itself then exit rather than putting the user into a shell inside a container with Verilator installed (where we could run the Verilator tests via 'lit'). So AFAICT we cannot use the docker container.

There is an argument for putting together a Docker image in which we compile and test. I've done this in the past and it's rather nice. We could actually set up a job to compile and fetch the docker image with a particular version of LLVM installed. This flow, however, is different from what most desktop developers do so IMO it's not worth the extra complexity.

[teqdruid]

@drom The Verilator image doesn't have the right packages to run lit and calling it per-test would be too complicated. So I'm gonna stick with the submodule for now.

[lattner]

You're planning to make verilator a submodule of circt?

[teqdruid]

Yes. I think that's the best way to tie CIRCT to a particular, recent version of verilator. Also, distros package fairly old versions. Some of the more recent bug fixes are necessary for me. The automated workflow will build (and cache it) just like llvm. The alternative is downloading a src tarball and compiling out of it, but that would make it more difficult for desktop developers to get a CIRCT-certified version compiled locally.

If we go the submodule route, I'm thinking we'll want a directory just for submodules ("/ext/verilator") so as to not clutter the top level.

[lattner]

Verilator is a complicated external open source project. Please don't make circt depend on (or submodule) it. Instead, we should provide instructions (and perhaps a script in circt/utils) for checking out and building verilator in the preferred way, and have the cmake logic detect it and enable conditional logic.

[teqdruid]

OK. I've already got detection logic (in my local branch) so it's not required to compile it. It's just a matter of where the source comes from.

[lattner]

I'd suggest making a docs/Verilator.md file that describes verilator support. You can include a recipe of the "right" way to build it so cmake, what you get, etc. You can see an example prior art in the llvm-project/llvm/docs/GoldPlugin.rst file. Thank you!

[teqdruid]

@lattner I don't think a full markdown is required (yet) since I'm providing a script to make it easier for users.

[lattner]

Nice test, thank you for staging this.

I'd really love for Stephen to review this, as I'm not really an expert on the build stuff at all.

[lattner]

Out of curiosity, why the || true?

[teqdruid]

verilator exits with a non-zero code (which fails the test). Is there better way to do this with lit or FileCheck?

[lattner]

Yeah, the preferred way is with the 'not' test, which verifies that it returns a non-zero exit code. something like this should work:

not verilator --cc --top-module main -Wall -Wpedantic %s | FileCheck %s