Change XLEN and FLEN to be configure-time options #870
Conversation
|
This is awesome! It looks like the upstream issues are fixed. Are there any other issues preventing this coming out of draft? |
test/riscv-tests/CMakeLists.txt
Outdated
| @@ -3,19 +3,19 @@ | |||
| # On success or failure they write to the `tohost` symbol. See this code: | |||
| # https://github.com/riscv/riscv-test-env/blob/4fabfb4e0d3eacc1dc791da70e342e4b68ea7e46/p/riscv_test.h#L200 | |||
|
|
|||
| file(GLOB elfs_rv32d CONFIGURE_DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/rv32*.elf") | |||
| file(GLOB elfs_rv64d CONFIGURE_DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/rv64*.elf") | |||
| # file(GLOB elfs_rv32d CONFIGURE_DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/rv32*.elf") | |||
There was a problem hiding this comment.
So I guess here we would need to edit the json?
Maybe the easiest way would be to make it a template and use configure_file() to replace @XLEN_LOG2@, etc. to generate a config_rv32d.json, config_rv64d.json in the build dir?
There was a problem hiding this comment.
Yeah. The first party tests already use jq to edit the JSON so maybe we just use that method here for now.
model/riscv_flen.sail
Outdated
| @@ -6,6 +6,10 @@ | |||
| /* SPDX-License-Identifier: BSD-2-Clause */ | |||
| /*=======================================================================================*/ | |||
|
|
|||
| type flen_bytes : Int = config base.flen_bytes | |||
|
|
|||
There was a problem hiding this comment.
Is it possible to automatically set this based on if F or D are supported instead of adding flen as a configuration option?
There was a problem hiding this comment.
Yeah, or the other way around. Not sure which is better. I agree we shouldn't have both.
Timmmm
force-pushed
the
user/timh/xlen_flen
branch
from
11d975a to
336ba9f
Compare
Getting closer but I hit another issue: rems-project/sail#1251 |
|
Also I need to:
And we'll need to wait for a new Sail compiler release after it's working. |
|
Ok I got the RV64 first party test passing! Still need to do all the todos I mentioned. |
|
I did have a crazy idea - what if we just take the xlen setting from the ELF file? :-D On balance, probably a bad idea. Explicitness is good. Just thought I'd mention it! |
Something to think about down the road maybe. I think we can technically even find out which extensions an elf was compiled with if we really wanted to. Might be nice to be able to just run an elf without thinking about the config. For verification purposes though (which I think is one of the primary uses of the Sail model), you want the model to exactly match an implementation no matter what the elf file being passed in is. Maybe there could be two different modes in the future: use the values from a config file if one is given and parse the elf if not. |
|
This basically works now. All the tests pass. Remaining todos:
|
This means you only need to compile the emulator once in order to support RV32F, RV32D, RV64F and RV64D.
Timmmm
force-pushed
the
user/timh/xlen_flen
branch
from
e3e76fa to
551e8f8
Compare
| @@ -167,8 +165,8 @@ endif() | |||
| include(CPack) | |||
|
|
|||
| # Convenience targets. | |||
| add_custom_target(csim DEPENDS riscv_sim_rv32d riscv_sim_rv64d) | |||
| add_custom_target(check DEPENDS generated_model_rv32d generated_model_rv64d) | |||
| add_custom_target(csim DEPENDS riscv_sim_rv) | |||
There was a problem hiding this comment.
One thing worth thinking about here is what we want the long term name of the simulator to be. I don't think we should change it more often than needed (it caused enough trouble when we added the d), so we should probably go with the new name now even though rvfi will need to be merged in later. The simplest answer would be to just go with riscv_sim, but I wonder if something like riscv_sail_sim might be better and less generic.
There was a problem hiding this comment.
Could also keep the old names around as scripts that run with a specific config file.
There was a problem hiding this comment.
If we make the config json a template, we could install some common default config files such as config_rv32f.json, config_rv64d.json.
And we'd be able to use them for the tests without jq
There was a problem hiding this comment.
@jordancarlin I agree, though it is a little awkward to do with how ${arch} currently works. If it is empty or something like _rvfi we'll get weird filenames in places, e.g. search for -o "${arch}".
If the simulator name could end with rv that would sneakily avoid the issue! sim_rv? :-D Probably not a name I would have chosen given a totally free choice tbf.
@arichardson Yeah... I dunno, it means you can't easily use them as starter configs.
Perhaps we could have a script that generates a wide variety of config files (like, 20 of them), and add them to the repository. CI can check they are up-to-date. Then it means they're easily visible in the source repo and users don't need whatever scripting thing we use to generate them.
This means you only need to compile the emulator once in order to support RV32F, RV32D, RV64F and RV64D.
This compiles doesn't work yet due to some issues in Sail:
configandconstraintrems-project/sail#1246There are also various TODOs but I thought I'd push this to prevent duplicated effort in case someone else starts working on it.