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

Q extension #445

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Q extension #445

wants to merge 1 commit into from

Conversation

jordancarlin
Copy link
Contributor

@jordancarlin jordancarlin commented Apr 5, 2024

Adds support for all Q extension instructions plus Q related Zfh and Zfa instructions. size_enc is also updated to have a 3 bit version (size_enc_wide) that supports QUAD word widths.

Fixes #187 from @LiuTaowen-Tony. There were lots of merge conflicts from the past 2 years. This resolves them and updates the changes that were made to comply with current conventions (removes effects, compatibility with existing Zfh and Zfa extensions that were added since the original version was written). It also changes the core soft-float interface to use two 64 bit values to avoid the need to create MPZ variables as was done in #187.

@jordancarlin jordancarlin marked this pull request as draft April 5, 2024 07:58
model/riscv_types.sail Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Apr 9, 2024

Test Results

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

Results for commit 39fc093. ± Comparison against base commit 8a2e0f5.

♻️ This comment has been updated with latest results.

@jordancarlin jordancarlin marked this pull request as ready for review April 17, 2024 04:54
Copy link
Collaborator

@Timmmm Timmmm left a comment

Choose a reason for hiding this comment

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

Haven't read it all but I skimmed some of it and it mostly looks reasonable. You may want to wait for #448 which will remove at least some of the changes needed. It's waiting for a new version of the Sail compiler to be released.

model/riscv_insts_aext.sail Outdated Show resolved Hide resolved
model/riscv_types.sail Outdated Show resolved Hide resolved
model/riscv_types.sail Outdated Show resolved Hide resolved
model/riscv_types.sail Outdated Show resolved Hide resolved
model/riscv_sys_control.sail Outdated Show resolved Hide resolved
model/riscv_insts_base.sail Outdated Show resolved Hide resolved
c_emulator/riscv_softfloat.c Outdated Show resolved Hide resolved
c_emulator/riscv_softfloat.c Outdated Show resolved Hide resolved
model/riscv_insts_aext.sail Outdated Show resolved Hide resolved
model/riscv_insts_base.sail Outdated Show resolved Hide resolved
@jordancarlin
Copy link
Contributor Author

jordancarlin commented Apr 21, 2024

You may want to wait for #448 which will remove at least some of the changes needed. It's waiting for a new version of the Sail compiler to be released.

@Timmmm It seems like the changes needed to either PR based on each other should be relatively minor, so it probably doesn't matter the order they are merged.

@jordancarlin
Copy link
Contributor Author

@Timmmm Thanks for all your earlier comments. I believe I’ve taken care of all of them at this point. I’d appreciate it if you could give this another look. Aside from more rigorous testing (which is being worked on) I think this PR is ready.

@@ -18,14 +18,15 @@ else
$(error '$(ARCH)' is not a valid architecture, must be one of: RV32, RV64)
endif

SAIL_FLEN := riscv_flen_D.sail
SAIL_FLEN := riscv_flen_Q.sail
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know if we want to change the default flen to 128. That's going to be a pretty uncommon configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you suggest adding a command line option? I think it would need to be a compile-time option if so. Or do we leave the makefile as is and have people manually change SAIL_FLEN before compiling if they want f128?

This issue will ideally go away once configuration via riscv-config is integrated, but since that might not be for a while, we probably need a solution until then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I think currently the best option would be to make it a Make variable, so people have to do

ARCH=RV64 FLEN=128 make ...

I don't think we want to parse riscv-config at compile time. The thing that would really help this is rems-project/sail#534 but there's no timeline for that so I think the Make variable is the way to go for now. We should have one for VLENMAX too really.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the Make variable is set to 64, it causes issues with the boxing/unboxing functions. Hopefully switching over to the generic version will solve this.

model/riscv_fdext_regs.sail:51.11-27:
51 |    128 if regval [127..16] ==  0x_FFFF_FFFF_FFFF_FFFF_FFFF_FFFF_FFFF
   |           ^--------------^ checking function argument has type bool('p)
   | Could not resolve quantifiers for subrange_bits
   | * (0 <= 16 & (16 <= 127 & 127 < 64))

if (sizeof(flen) == 32)
then 0x_FFFF @ val_16b
else 0x_FFFF_FFFF_FFFF @ val_16b
match (sizeof(flen)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These boxing/unboxing functions can at least be generic, as in my other commit.

However, unless you are in a hurry for this I kind of feel like it would still be worth:

  1. Waiting for the new float support in the Sail standard library.
  2. Update my other MR to use that & make the code more generic.
  3. Then update this.

That may take a while I'm afraid but I guess you'd noticed that's pretty normal for this project! (Also I'm not the boss here so feel free to seek a second opinion!)

I think we should re-evaluate when there's a new Sail compiler release, which I imagine shouldn't be too long.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know there is work being done on quad tests for riscv-arch-test which will want to use the sail model to generate signatures. If those are completed before we get all of the floating point updates into the Sail standard library and make the code more generic, then we may want to revisit merging this sooner but otherwise, I agree.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jordancarlin Do you know if anyone is planning to make and tape-out a real chip that implements the Q extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@martinberger The OpenHW Core-V-Wally processor (https://github.com/openhwgroup/cvw) implements the Q extension in some of its configurations. There is an upcoming tape-out planned for it very soon. I'm double checking which configuration is planned for that, but it sounds like it might include Q.

Complete Q extension implementation including Zfa instructions

Based on original work by 'LiuTaowen-Tony' in riscv#187
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