Skip to content

Fixed Coverage Holes for RV32D #673

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

Merged
merged 2 commits into from
Jul 28, 2025

Conversation

sudo-apt-abdullah
Copy link
Contributor

This PR resolves a coverage drop in the RV32D extension caused by incorrect operand values in the associated datasets. Each test appends a dataset at the end, but the values were mistakenly NAN_BOXED, even for 64-bit double-precision operands. For example, the dataset boxed op1val: 0x7f4bab861e3ffdaf as NAN_BOXED(0x7f4bab861e3ffdaf172613658591624623, 64, FLEN), which was invalid and introduced noise due to appending unrelated bits.

In RV32D, since both the source and destination operands are already 64-bit, boxing is unnecessary. The faulty boxing logic ended up pulling only the lower 32 bits and extending them incorrectly, making the operands useless for coverage tracking.

To fix this, I wrote a script that parses each test file, extracts the actual operand values used during execution, and replaces the dataset entries with these corrected values. This ensures consistency between test behavior and dataset expectations, allowing coverage bins to be hit accurately.

Additionally, I updated the fp_dataset to exclude rounding mode (rm_val) coverpoints for fcvt.d.w, fcvt.d.wu, and fcvt.d.s, as these instructions either produce exact results or do not round, rendering rm_val irrelevant.

Overall, this improves the fidelity of the coverage model for RV32D without needing to regenerate the tests, as the issue was isolated to the dataset formatting, not the test logic itself.

@allenjbaum
Copy link
Collaborator

allenjbaum commented Jul 1, 2025 via email

@sudo-apt-abdullah
Copy link
Contributor Author

sudo-apt-abdullah commented Jul 1, 2025

You're right—GitHub struggles to open the test files due to their size, but the changes across them are minimal and localized. The only substantive update is in the .py script, where I excluded the irrelevant rm_val coverpoints for fcvt.d.w, fcvt.d.wu, and fcvt.d.s, since these conversions are either exact or don't involve rounding.

As for the test files, the format and test cases remain entirely unchanged. The only modification is to the dataset block at the end of each file, where I updated the operand values to reflect the actual values used during execution. This was necessary to correct previous boxing errors and ensure coverage bins are accurately hit.

You can randomly inspect any of the test files—the changes are restricted to the dataset section, and everything else is untouched. This should make the merge straightforward.

@allenjbaum
Copy link
Collaborator

allenjbaum commented Jul 1, 2025 via email

@sudo-apt-abdullah
Copy link
Contributor Author

sudo-apt-abdullah commented Jul 1, 2025

If it works for Sir @UmerShahidengr, otherwise I can show the my local changes to Sir when I meet him in person.

@allenjbaum
Copy link
Collaborator

allenjbaum commented Jul 1, 2025 via email

@UmerShahidengr
Copy link
Collaborator

I have checked these changes from @sudo-apt-abdullah system directly, and it looks fine to me. A python script is doing the same change in all tests, no other change was observed in any test. So I am merging this one. Merging the #670 has introduced the conflicts in this one. @sudo-apt-abdullah please resolve those conflicts, and I will merge that one

Copy link
Collaborator

@UmerShahidengr UmerShahidengr left a comment

Choose a reason for hiding this comment

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

LGTG

@UmerShahidengr UmerShahidengr merged commit 18e0cab into riscv-non-isa:dev Jul 28, 2025
2 checks passed
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.

3 participants