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

Fixes to the FPU for issue #791 #794

Merged
merged 14 commits into from Feb 14, 2024
Merged

Fixes to the FPU for issue #791 #794

merged 14 commits into from Feb 14, 2024

Conversation

mikaelsky
Copy link
Collaborator

@mikaelsky mikaelsky commented Feb 7, 2024

This is a bit bigger patch to the FPU. There might be one more small patch coming for fmin/fmax and or the tests where Sail disagrees with the core and Imperas. Most, if not all, of these are differences in subnormal handling. We flush to 0, Sail does not.

The following test are still being debugged. Seems to be a reference simulator issue.
block_sim/rv32i_m_Zfinx_fmax_b1
block_sim/rv32i_m_Zfinx_fmax_b19
block_sim/rv32i_m_Zfinx_fmin_b1
block_sim/rv32i_m_Zfinx_fmin_b19

This test still fails 6 instruction tests out of 209 against the Sail reference, but passes our Imperas reference.
rv32i_m_Zfinx_fcvt.w.s_b28
The failure is for non-subnormal, positive but low exponent floating point numbers. Where Sail flags NX and we do not. The actual resulting integer Sail, Imperas and the core all agree is correct.

With this change the following riscvof (F->Zfinx) test all pass:
block_sim/rv32i_m_Zfinx_fadd_b1
block_sim/rv32i_m_Zfinx_fadd_b10
block_sim/rv32i_m_Zfinx_fadd_b11
block_sim/rv32i_m_Zfinx_fadd_b12
block_sim/rv32i_m_Zfinx_fadd_b13
block_sim/rv32i_m_Zfinx_fadd_b2
block_sim/rv32i_m_Zfinx_fadd_b5
block_sim/rv32i_m_Zfinx_fadd_b7
block_sim/rv32i_m_Zfinx_fclass_b1
block_sim/rv32i_m_Zfinx_fcvt.s.w_b25
block_sim/rv32i_m_Zfinx_fcvt.s.w_b26
block_sim/rv32i_m_Zfinx_fcvt.s.wu_b25
block_sim/rv32i_m_Zfinx_fcvt.s.wu_b26
block_sim/rv32i_m_Zfinx_fcvt.w.s_b27
block_sim/rv32i_m_Zfinx_fcvt.wu.s_b27
block_sim/rv32i_m_Zfinx_fsub_b1
block_sim/rv32i_m_Zfinx_fsub_b11
block_sim/rv32i_m_Zfinx_fsub_b13
block_sim/rv32i_m_Zfinx_fsub_b2
block_sim/rv32i_m_Zfinx_fsub_b3
block_sim/rv32i_m_Zfinx_fsub_b5
block_sim/rv32i_m_Zfinx_fsub_b7
block_sim/rv32i_m_Zfinx_feq_b1
block_sim/rv32i_m_Zfinx_feq_b19
block_sim/rv32i_m_Zfinx_flw-align
block_sim/rv32i_m_Zfinx_fle_b1
block_sim/rv32i_m_Zfinx_fle_b19
block_sim/rv32i_m_Zfinx_flt_b1
block_sim/rv32i_m_Zfinx_flt_b19
block_sim/rv32i_m_Zfinx_fsgnj_b1
block_sim/rv32i_m_Zfinx_fsgnjn_b1
block_sim/rv32i_m_Zfinx_fsgnjx_b1
block_sim/rv32i_m_Zfinx_fsw-align
block_sim/rv32i_m_Zfinx_fadd_b3
block_sim/rv32i_m_Zfinx_fsub_b12
block_sim/rv32i_m_Zfinx_fadd_b4
block_sim/rv32i_m_Zfinx_fadd_b8
block_sim/rv32i_m_Zfinx_fsub_b4
block_sim/rv32i_m_Zfinx_fsub_b8
block_sim/rv32i_m_Zfinx_fcvt.w.s_b1
block_sim/rv32i_m_Zfinx_fcvt.w.s_b22
block_sim/rv32i_m_Zfinx_fcvt.w.s_b23
block_sim/rv32i_m_Zfinx_fcvt.w.s_b24
block_sim/rv32i_m_Zfinx_fcvt.w.s_b28
block_sim/rv32i_m_Zfinx_fcvt.w.s_b29
block_sim/rv32i_m_Zfinx_fcvt.wu.s_b1
block_sim/rv32i_m_Zfinx_fcvt.wu.s_b22
block_sim/rv32i_m_Zfinx_fcvt.wu.s_b23
block_sim/rv32i_m_Zfinx_fcvt.wu.s_b24
block_sim/rv32i_m_Zfinx_fcvt.wu.s_b29
block_sim/rv32i_m_Zfinx_fmul_b1
block_sim/rv32i_m_Zfinx_fmul_b2
block_sim/rv32i_m_Zfinx_fmul_b3
block_sim/rv32i_m_Zfinx_fmul_b4
block_sim/rv32i_m_Zfinx_fmul_b5
block_sim/rv32i_m_Zfinx_fmul_b6
block_sim/rv32i_m_Zfinx_fmul_b7
block_sim/rv32i_m_Zfinx_fmul_b8
block_sim/rv32i_m_Zfinx_fmul_b9

Signed-off-by: Mikael Mortensen <119539842+mikaelsky@users.noreply.github.com>
Signed-off-by: Mikael Mortensen <119539842+mikaelsky@users.noreply.github.com>
Signed-off-by: Mikael Mortensen <119539842+mikaelsky@users.noreply.github.com>
@stnolting stnolting added bug Something isn't working risc-v compliance Modification to comply with official RISC-V specs. HW hardware-related labels Feb 9, 2024
@stnolting stnolting linked an issue Feb 9, 2024 that may be closed by this pull request
@mikaelsky
Copy link
Collaborator Author

Just did a merge in my fork, hopefully I didn't screw this on up :(

Signed-off-by: Mikael Mortensen <119539842+mikaelsky@users.noreply.github.com>
@stnolting
Copy link
Owner

Just did a merge in my fork, hopefully I didn't screw this on up :(

Seems fine ;)

I have been trying to work through all of your edits. Thanks for providing so much details in #791! I will do some tests on my side and run some synthesis experiments before merging this.

@stnolting
Copy link
Owner

stnolting commented Feb 11, 2024

Synthesis looks good, area increment is minimal and there is no impact on the core's critical path. So far so good! I just made some minor code edits.

I also did some tests including the floating point tests. After some updates everything works fine. I just found one error (in 24.000.000 test cases):

#11: FCVT.W.S (float to signed integer)...
33740: opa = 0xf3800000, opb = 0x00000000 : ref[SW] = 0x80000000 vs. res[HW] = 0x00000000 [FAILED]
Errors: 1/1000000 [FAILED]

I have not yet investigated this further. I think this is just another corner case. However, your patches are great am I am ok with merging this. 🚀

@stnolting stnolting changed the title Fixes to the FPU for isse #791 Fixes to the FPU for issue #791 Feb 11, 2024
@stnolting stnolting merged commit 897667f into stnolting:main Feb 14, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working HW hardware-related risc-v compliance Modification to comply with official RISC-V specs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FPU more fflags issues and a few logic bugs
2 participants