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

Add support for RISCOF based checks #1251

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

towoe
Copy link
Contributor

@towoe towoe commented Jan 25, 2021

This PR addresses #1233 and provides some basic functionality to run riscv-compliance checks based on RISCOF.
RISCOF is still in development and the switch to it in riscv-compliance is only partially completed.

Add a Python model of Ibex to integrate with RISCOF.
Add a pseudo model to compare the output of Ibex to reference signatures.
Run the check in CI, it is intended not to fail even though some tests will not pass. The same specification is used for different configurations of Ibex, this needs to be extended in the future. (Provide more ibex_*_isa.yaml files.)

@towoe
Copy link
Contributor Author

towoe commented Jan 25, 2021

Anyone know what would be the correct solution to ERROR: Cannot uninstall 'PyYAML'. It is a distutils installed project and thus we cannot accurately determine which files belong to it which would lead to only a partial uninstall.? Uninstall it, use pip --ignore-installed?

@rswarbrick
Copy link
Contributor

For the pyYAML problem, I think you probably just want to use the distro version. See PR #1226.

@towoe
Copy link
Contributor Author

towoe commented Jan 25, 2021

Sorry, I should have explained it in more detail. pyYAML is pulled in as a dependency from riscof which I specified. How do I say that the distro version should be used in this case?

@rswarbrick
Copy link
Contributor

Oh, I see. I guess --ignore-installed makes sense (but possibly with a comment explaining what's going on?)

@towoe
Copy link
Contributor Author

towoe commented Jan 25, 2021

-I, --ignore-installed
              Ignore  the installed packages, overwriting them. This can break your system if the existing package is of a different version or was installed with
              a different package manager!

reading the documentation more carefully, maybe the safer bet would be remove the local installation again?
But I don't really understand why pyYAML is even needed/updated if it is already available?

@towoe
Copy link
Contributor Author

towoe commented Jan 25, 2021

Okay, I just saw in the log that pyYAML 3.12 is available, that's from 2016. I did not directly find which package requires what, but I would guess somewhere a newer version is required. Soo, remove system installation? Maybe @imphil has an opinion here?

@imphil
Copy link
Contributor

imphil commented Jan 25, 2021

Yes, we explicitly installed the distribution's version of pyyaml as the PyPi-distributed version didn't include the libyaml (C) library bindings, but only the pure-Python version, which is significantly slower. Ubuntu 18.04 gives us pyyaml 3.x.

The requirement for a newer pyyaml version comes from riscv-config (https://github.com/riscv/riscv-config/blob/master/setup.py#L38). It's hard to tell where the exact version requirement came from looking at the relevant commit (riscv-software-src/riscv-config@4617b31).

But at the same time I just saw that pyyaml is now installing wheels for Linux which include the libyaml bindings, so we actually don't need to use the distribution package any more.

You can therefore remove the python3-yaml distribution package from the list of packages, and pip should pick up a compatible version from PyPi.

@towoe
Copy link
Contributor Author

towoe commented Jan 25, 2021

Thanks a lot Philipp for your investigation!

@towoe
Copy link
Contributor Author

towoe commented Jan 25, 2021

Should pip install update the packages again? pip install -U or was there another reason why it was removed?

@imphil
Copy link
Contributor

imphil commented Jan 25, 2021

The update flag shouldn't be needed, we're happy with any version satisfying the requirements.

@towoe towoe force-pushed the add-riscof-check branch 2 times, most recently from 90a0ae9 to 859d3d1 Compare January 26, 2021 07:20
RISC-V Compliance reference model to compare existing signatures.
Reference signatures created with Spike RISC-V ISA RISCOF plugin.
Run riscv-compliance v2 tests, but ignore failures.

RISCOF based compliance check is still in active development and some
tests are known to fail.
@vogelpi
Copy link
Contributor

vogelpi commented Sep 1, 2021

Hey @towoe , @imphil , @rswarbrick ,
more than 7 months have passed since the last activity in this PR. Do you know what the status of this PR is? Are there open points to be addressed, things that need to be discussed?

@towoe
Copy link
Contributor Author

towoe commented Sep 2, 2021

Hi @vogelpi,
as far as I can remember the new version of the compliance framework was not ready when I created the PR.
This PR shows how the new compliance checks could be realized and should make it easy to rebase or "re-implement" it.
I wasn't sure if this PR is wanted, but if there is a need I could at some point revisit this topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants