Use C++ Sail output - #1274
Conversation
4ca766a to
4519eeb
Compare
Tim Hutt (Timmmm)
added
the
pending sail compiler
|
This is probably unrelated for now, but enabling multi-hart simulation would affect the Debug Module we are working on. So far, all of our code has been written in Sail (since it makes handling the many registers and bitfields easier), but I think parts of the Debug Module would need to be moved back to C++ to properly support 1+N harts. |
|
That's a good point. I was sort of vaguely aware of that. I guess in theory you could still write it in Sail as long as it was a completely separate Sail model, and then you'd need to connect them somehow. |
Yeah, that would be perfect if this was possible! We can revisit this topic once this PR is merged, along with the debug stuff. |
4519eeb to
29658fa
Compare
29658fa to
ce4448f
Compare
ce4448f to
087f5de
Compare
0595c87 to
1d5c593
Compare
Tim Hutt (Timmmm)
removed
the
pending sail compiler
Jordan Carlin (jordancarlin)
left a comment
Jordan Carlin (jordancarlin)
left a comment
There was a problem hiding this comment.
Some initial comments. Will review in more detail later.
I also think it might be worth renaming the c_emulator directory to cpp_emulator.
Or just |
I think I originally did that when the OCaml emulator was removed but people wanted to keep the |
88e8e3e to
20f1452
Compare
20f1452 to
89aa798
Compare
|
Rebased. That was fairly painful so I'm afraid I had to remove the reservation printing option. I don't think it's too bad since it didn't really exist until very recently. We can add it back later. It would be good to get this merged fairly soon to avoid more painful rebases! |
Alexander Richardson (arichardson)
left a comment
Alexander Richardson (arichardson)
left a comment
There was a problem hiding this comment.
Seems fine to me. Thanks for the hard work on adding C++ output!
89aa798 to
67600fe
Compare
Jordan Carlin (jordancarlin)
left a comment
Jordan Carlin (jordancarlin)
left a comment
There was a problem hiding this comment.
Couple of minor nits, but overall looks great. Make sure to update all of the references in the README that talk about the C emulator/generated C code.
I'd also still like to see the c_emulator directory renamed to either cpp_emulator or just emulator.
| @@ -6,7 +6,7 @@ | |||
| // Generated in the model C code. This is a simple test runner that just | |||
There was a problem hiding this comment.
| // Generated in the model C code. This is a simple test runner that just | |
| // Generated in the model C++ code. This is a simple test runner that just |
| // The Model class derives from this one so when Sail calls C callback | ||
| // functions it actually calls methods of this class. However they are | ||
| // virtual functions so they actually call the Platform implementations | ||
| // (see riscv_platform_impl.h). It's done this way because: |
There was a problem hiding this comment.
riscv_model_impl.h? There is no riscv_platform_impl.h. And if so, should we rename this file to riscv_model_if.h (or vice versa)?
There was a problem hiding this comment.
Oops yeah should have been riscv_model_impl.h. I think platform is still an ok name though because this is an interface to the platform for the model... if that makes sense?
There was a problem hiding this comment.
Fine either way, but I think it would be easier to follow the logic if the interface and implementation names match.
There was a problem hiding this comment.
Pull request overview
This PR migrates the RISC-V Sail model from C to C++ output, enabling multiple model instances for multicore simulation. The Sail-generated code is now wrapped in a hart::Model class derived from PlatformInterface, allowing flexible platform callback implementations through virtual methods. The ModelImpl class provides the concrete platform implementation and manages callback registration.
Key changes:
- Sail external function declarations changed from
c:tocpp:target - Model generated as C++ class in
hartnamespace, derived fromPlatformInterface - Platform callbacks implemented as virtual methods in
ModelImpl
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| model/**/*.sail | Updated external function declarations from c: to cpp: target |
| model/CMakeLists.txt | Changed Sail compiler to generate C++ with namespace and class derivation |
| c_emulator/riscv_platform_if.h | New platform interface base class with virtual callback methods |
| c_emulator/riscv_model_impl.h/cpp | New model implementation with platform callbacks and helper methods |
| c_emulator/riscv_callbacks_if.h | Updated callback interface to accept Model reference parameter |
| c_emulator/riscv_callbacks_*.cpp | Updated callback implementations to use Model reference |
| c_emulator/riscv_softfloat.h/cpp | Removed extern "C" wrapping for C++ compatibility |
| c_emulator/riscv_sim.cpp | Updated to use global g_model instance and new API |
| c_emulator/rvfi_dii.h/cpp | Updated RVFI handler to use Model instance and member function pointers |
| c_emulator/CMakeLists.txt | Updated build to compile .cpp instead of .c for generated model |
| test/unit_tests/main_unit_tests.cpp | Removed extern "C" and init_sail_configured_types() call |
| doc/ReadingGuide.md | Marked documentation as needing update |
| c_emulator/riscv_*.h/cpp (deleted) | Removed old C-style platform and callback implementations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Oops. Seems like I hit the wrong button and triggered a review from Copilot. Feel free to disregard. |
This switches from using Sail's C output to using C++. The output code gets wrapped in a `class`, which means we can create more than one instance of it (e.g. for multicore). The `Model` class is currently *not* thread safe due to the use of temporary globals in `sail.c`. So although you can simulate multicore systems, you have to execute them one at a time. The handling of platform callbacks is a little convoluted but I think this is a quite flexible solution. Sail is instructed to derive the `Model` class from `PlatformImpl` which has virtual methods for all of the platform callbacks with default nop implementations. This means the default `Model` can be constructed and has a "nop" platform. Then we add our implementation of the platform with `ModelImpl` which overrides those methods. `ModelImpl` also allows registering callback receivers. This is not 100% perfect yet. We still have a global `g_model`, which is not ideal.
67600fe to
0ba2878
Compare
|
It caught a couple of very minor issues at least... |
Nadime Barhoumi (nadime15)
left a comment
Nadime Barhoumi (nadime15)
left a comment
There was a problem hiding this comment.
I tried my best to review it. I am a C++ noob, but it looks good to me! :D
Co-authored-by: Nadime Barhoumi <nadime@riscv.org> Signed-off-by: Tim Hutt <tdhutt@gmail.com>
Alexander Richardson (arichardson)
left a comment
Alexander Richardson (arichardson)
left a comment
There was a problem hiding this comment.
Probably good to get this merged ASAP to avoid further conflicts.
Jordan Carlin (jordancarlin)
left a comment
Jordan Carlin (jordancarlin)
left a comment
There was a problem hiding this comment.
I think we should merge this and then we can follow up with smaller PRs for any other improvements.
|
Ok I'll give it a good few days, seeing as this is a pretty huge change, and then merge it. |
Tim Hutt (Timmmm)
added
the
will be merged
little bit off topic. Just wondering where is the button? Want to try in #1370 |
In the top right where you can request reviews from anyone else. |
This switches from using Sail's C output to using C++. The output code gets wrapped in a
class, which means we can create more than one instance of it (e.g. for multicore).The
Modelclass is currently not thread safe due to the use of temporary globals insail.c. So although you can simulate multicore systems, you have to execute them one at a time.The handling of platform callbacks is a little convoluted but I think this is a quite flexible solution. Sail is instructed to derive the
Modelclass fromPlatformImplwhich has virtual methods for all of the platform callbacks with default nop implementations.This means the default
Modelcan be constructed and has a "nop" platform. Then we add our implementation of the platform withModelImplwhich overrides those methods.ModelImplalso allows registering callback receivers.This is not 100% perfect yet. We still have a global
g_model, which is not ideal.