Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support verilator in clash-testsuite #2019

Merged
merged 3 commits into from
Jan 25, 2022
Merged

Conversation

alex-mckenna
Copy link
Contributor

@alex-mckenna alex-mckenna commented Dec 8, 2021

This PR adds support for using verilator in (System)Verilog tests in the testsuite. This largely consists of changes to black boxes which verilator does not support to include C++ definitions which can be used for simulation instead.

Still TODO:

  • Write a changelog entry (see changelog/README.md)
  • Check copyright notices are up to date in edited files
  • Update black boxes for clockGen when the tbClockGen black box is accepted

@alex-mckenna alex-mckenna force-pushed the clash-testsuite-verilator branch 3 times, most recently from 5d4a6b9 to 20aed6b Compare December 9, 2021 14:37
@alex-mckenna alex-mckenna marked this pull request as ready for review December 9, 2021 14:43
@alex-mckenna alex-mckenna force-pushed the clash-testsuite-verilator branch 3 times, most recently from a42d822 to 3ff309d Compare December 10, 2021 12:36
Copy link
Member

@martijnbastiaan martijnbastiaan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall

tests/src/Test/Tasty/Clash.hs Show resolved Hide resolved
tests/src/Test/Tasty/Clash.hs Outdated Show resolved Hide resolved
tests/src/Test/Tasty/Ghdl.hs Outdated Show resolved Hide resolved
tests/src/Test/Tasty/Verilator.hs Outdated Show resolved Hide resolved
Comment on lines +449 to +439
-- TODO: Since tasty doesn't provide one, we should really provide a better
-- set of combinators for describing test dependencies. That way we can have
-- some more principled way of having a test structure like
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's definitely Tasty's weakest point. Basically we already work our way around it by passing down names so we can construct the absolute name of the test at test location. It would be some much more intuitive if you could just do something like: TestGroupSeq [ ... ]. But alas..

Comment on lines 391 to 424
-- If we are generating (System)Verilog, we could potentially verilate
-- the results. Clash can output a C++ shim for doing this automatically.
fileNames2 <-
case hdl of
VHDL -> pure fileNames1
_ -> writeVerilatorShim hdlDir topNm fileNames1

writeManifest manPath manifest0{fileNames=fileNames2}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love for this to become a separate executable someday. It feel icky to have this all dumped in Driver.

Alex McKenna added 3 commits January 25, 2022 16:13
It is now possible to selectively enable tests which use a
specific vendor tool by passing a flag to the testsuite driver,
e.g. --verilator for running tests with verilator. If a tool is not
given it's tests pass (as the tasty API does not allow deciding to
skip a test inside IsTest.run). If the tool is not found the error
is not tidied up instead of showing the contents of stderr.

This is a somewhat crude first attempt at addressing #2012.
Copy link
Member

@martijnbastiaan martijnbastiaan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noice :)

@martijnbastiaan martijnbastiaan merged commit 5f7d074 into master Jan 25, 2022
@martijnbastiaan martijnbastiaan deleted the clash-testsuite-verilator branch January 25, 2022 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants