-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[CI][BYORTL] add Verilator regression test to CI #7098
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @vegaluisjose LGTM
Thanks @vegaluisjose , looking at the files, looks like that It would be great to not do so, and build the library before test (at the same stage) |
Because the current mainline does not generate libverilator. The caching step will fail because we cannot found libverilator. Sorry that i did not point in previous code review, and happy to discuss this further. Right now this PR has the following dependency:
It would really be nice if the hw part can be built independently, and loaded on demand. So libtvm can load select and load the right version of libverilator when necessary. We can build the libverilator right at the test stage, with libtvm available. We can explore a few ways to do that. One way I can think about, is as follows:
Happy to chat more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preventing accidental merges
+1 |
198f5ed
to
75b565a
Compare
Hey guys, The CI is updated now with the new version of Verilator backend. CI (Jenkins) seems to be working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great, thanks for making sure we land these verilator CI tests @vegaluisjose !
Thank you @liangfu @tqchen @vegaluisjose ; the PR has been merged. |
@tqchen @tmoreau89