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

Remove the OCaml emulator #544

Merged
merged 4 commits into from
Sep 19, 2024

Conversation

Timmmm
Copy link
Collaborator

@Timmmm Timmmm commented Sep 9, 2024

This has the following benefits:

  • Reduces the maintenance burden for making changes to the emulator. They don't have to be done in two places and you don't need to learn OCaml.
  • Removes the dependency on OCaml and OPAM, which is a weirdly bug-prone tool.
  • Removes confusion for new users about which emulator to use.
  • Shorter CI time.

Note, I left the targets to actually build the OCaml code since I believe it is used by TestRig for its random instruction generation.

Fixes #509

Copy link

github-actions bot commented Sep 9, 2024

Test Results

396 tests   - 316   396 ✅  - 316   0s ⏱️ ±0s
  4 suites  -   2     0 💤 ±  0 
  1 files   ±  0     0 ❌ ±  0 

Results for commit 842bc30. ± Comparison against base commit c61351e.

This pull request removes 316 tests.
Building 32-bit RISCV OCaml emulator
Building 64-bit RISCV OCaml emulator
OCaml-32 rv32mi-p-breakpoint.elf
OCaml-32 rv32mi-p-csr.elf
OCaml-32 rv32mi-p-illegal.elf
OCaml-32 rv32mi-p-ma_addr.elf
OCaml-32 rv32mi-p-ma_fetch.elf
OCaml-32 rv32mi-p-mcsr.elf
OCaml-32 rv32mi-p-sbreak.elf
OCaml-32 rv32mi-p-scall.elf
…

♻️ This comment has been updated with latest results.

@Alasdair
Copy link
Collaborator

Alasdair commented Sep 9, 2024

The Sail to OCaml translation could also use quite a few changes, as it's the first thing I ever implemented when I started working on Sail. It still uses ocamlbuild for example, whereas the OCaml ecosystem has now mostly move to dune.

Removing the use here would actually free me to make some much needed changes.

@Timmmm Timmmm force-pushed the user/timh/remove_ocaml_emulator branch from c7d03e8 to 8371d44 Compare September 9, 2024 15:05
@Timmmm
Copy link
Collaborator Author

Timmmm commented Sep 9, 2024

@PeterRugg This PR removes OCaml support from the model, which I think would affect TestRig. Is that a deal breaker? What's the status of TestRig?

@jrtc27
Copy link
Collaborator

jrtc27 commented Sep 9, 2024

We use an independent Haskell generator these days (https://github.com/CTSRD-CHERI/QuickCheckVEngine). I don’t know if the old Sail model-based OCaml one even still works, likely not, it hasn’t been touched in 5 years.

@PeterRugg
Copy link
Contributor

Yeah, we certainly don't compare against the ocaml model, and can't see any reason why we would. Losing the sail-based test generation is sad, but as @jrtc27 says, we never really got chance to investigate using it properly.

Copy link
Collaborator

@arichardson arichardson left a comment

Choose a reason for hiding this comment

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

I believe the one thing we are losing here is the ability to generate the DTB for the OS boot. I wonder if this should be added to the C simulator first? Otherwise I am more than happy to see the ocaml simulator being removed since it makes it a lot easier to add new extensions with hooks.

@arichardson
Copy link
Collaborator

I believe the one thing we are losing here is the ability to generate the DTB for the OS boot. I wonder if this should be added to the C simulator first? Otherwise I am more than happy to see the ocaml simulator being removed since it makes it a lot easier to add new extensions with hooks.

Looks like it's just a simple string template (let spike_dts isa_spec mmu_spec cpu_hz insns_per_rtc_tick mems), so should be easy to move over to C.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Sep 10, 2024

I have actually implemented that for our C++ emulator so I'll port it over.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Sep 16, 2024

I have actually implemented that for our C++ emulator so I'll port it over.

I actually haven't implemented it - I was thinking of something else. It is pretty simple; just generate a string template and then run dtc to compile it.

But doing that in C is a right pain, so I'm voting we defer this and add it again if/when somebody asks for it.

If you want to boot Linux you can still manually make a DTB.

@arichardson
Copy link
Collaborator

I have actually implemented that for our C++ emulator so I'll port it over.

I actually haven't implemented it - I was thinking of something else. It is pretty simple; just generate a string template and then run dtc to compile it.

But doing that in C is a right pain, so I'm voting we defer this and add it again if/when somebody asks for it.

If you want to boot Linux you can still manually make a DTB.

Seems fine to me. It would be nice to generate this so that operating systems can parse it and boot, but can be done as a follow-up when needed.

@Timmmm Timmmm marked this pull request as ready for review September 16, 2024 20:41
This has the following benefits:

* Reduces the maintenance burden for making changes to the emulator. They don't have to be done in two places and you don't need to learn OCaml.
* Removes the dependency on OCaml and OPAM, which is a weirdly bug-prone tool.
* Removes confusion for new users about which emulator to use.
* Shorter CI time.

Note, I left the targets to actually build the OCaml code since I believe it is used by TestRig for its random instruction generation.

Fixes riscv#509
This will allow the OCaml backend to be improved in the Sail compiler.
Since removing OCaml support, the callback name is the same for all targets, so I switched to the simpler syntax.
@Timmmm Timmmm force-pushed the user/timh/remove_ocaml_emulator branch from 8371d44 to 2335e9d Compare September 16, 2024 20:50
Copy link
Contributor

@jordancarlin jordancarlin left a comment

Choose a reason for hiding this comment

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

LGTM

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Co-authored-by: Alexander Richardson <mail@alexrichardson.me>
Signed-off-by: Tim Hutt <tdhutt@gmail.com>
@Timmmm
Copy link
Collaborator Author

Timmmm commented Sep 17, 2024

I'll merge this in a few days if nobody objects.

@Timmmm Timmmm merged commit bdf95b0 into riscv:master Sep 19, 2024
2 checks passed
@Timmmm Timmmm deleted the user/timh/remove_ocaml_emulator branch September 23, 2024 09:56
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.

Remove OCaml emulator
6 participants