riscv / sail-riscv

Sail RISC-V model
https://lists.riscv.org/g/tech-golden-model
Other
407 stars 148 forks source link

Sail Crosscheck Testing with ACT Against Spike #481

Open Abdulwadoodd opened 1 month ago

Abdulwadoodd commented 1 month ago

This PR should complete this SOW.

PR adds the RISCOF-based CI infrastructure to test sail-riscv against Spike using ACTs. Both Sail and Spike are configured using ISA yaml configuration (provided under ci-tests/riscof/) which are validated by riscv-config using RISCOF

Current infrastructure uses 3.8.10 release of arch-test-suites. The later releases of arch-tests (that add Zfa and Zfh tests) are partially broken for RISCOF - and should be fixed shortly.

CI runs ACTS in RV32I, RV64I, and RV32E configurations. Details are as follows. After successful completion, test reports for each configuration are uploaded to GitHub Workspace.

Kindly review, and make suggestions how the CI should be pulled in to this repo. I'll be happy to make the requested changes.

jrtc27 commented 1 month ago

Most of this looks like it belongs in a centralised location so code can be reused?

jrtc27 commented 1 month ago

That or each project should put its riscof config in a place that other projects can use. The Sail model shouldn’t have to have a copy of how to build tests for Spike.

Abdulwadoodd commented 1 month ago

this looks like it it belongs in a centralised location

Not exactly sure what this means but most of the code (python plugins, linkers, model-specific headers or yaml, etc.) here is either produced while setting up RISCOF (configuring Spike as DUT and SAIL as reference model) or added/modified manually as per the requirement by following the RISCOF documentation.

Each project should put its riscof config in a place that other projects can use

What do you mean by "project" here?

jrtc27 commented 1 month ago

this looks like it it belongs in a centralised location

Not exactly sure what this means but most of the code (python plugins, linkers, model-specific headers or yaml, etc.) here is either produced while setting up RISCOF (configuring Spike as DUT and SAIL as reference model) or added/modified manually as per the requirement by following the RISCOF documentation.

I don't really care about the background, I care about the end result, which does not look like good software engineering to me.

Each project should put its riscof config in a place that other projects can use

What do you mean by "project" here?

The Sail model, the Spike simulator, etc.

Alasdair commented 1 month ago

If we want to run ACT cross-checks against spike in our CI, which I think is a good idea, then at least some of the infrastructure for running that needs to be in this repository. While we could pull some of that in from a third-party repository, that isn't necessarily a good idea as it means the CI scripts/config would no-longer be versioned together with the model which creates a bunch of issues.

Overall, I think it would be better if the ci-tests directory was moved into the existing tests folder as a subdirectory and renamed (the existing tests are already CI tests!), maybe as act_cross_check or similar. I'd also change the formatting of cSim to just csim, I don't see any reason for the weird capitalisation of the the 'S'.

jrtc27 commented 1 month ago

If we want to run ACT cross-checks against spike in our CI, which I think is a good idea, then at least some of the infrastructure for running that needs to be in this repository. While we could pull some of that in from a third-party repository, that isn't necessarily a good idea as it means the CI scripts/config would no-longer be versioned together with the model which creates a bunch of issues.

Overall, I think it would be better if the ci-tests directory was moved into the existing tests folder as a subdirectory and renamed (the existing tests are already CI tests!), maybe as act_cross_check or similar. I'd also change the formatting of cSim to just csim, I don't see any reason for the weird capitalisation of the the 'S'.

The Sail side should be, but the Spike side shouldn't, otherwise those files won't be versioned with Spike, creating the same kinds of issues you talk about.