-
Notifications
You must be signed in to change notification settings - Fork 232
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
Fixed Coverage Holes for RV32D #673
Conversation
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. |
I can't look at any of the changes for the D extension on github; I'm not
sure I can even add a review....
I could look at fclass (not sure of any of the other files) and it looks
correct.
…On Tue, Jul 1, 2025 at 8:38 AM sudo-apt-abdullah ***@***.***> wrote:
*sudo-apt-abdullah* left a comment (riscv-non-isa/riscv-arch-test#673)
<#673 (comment)>
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.
Feel free to 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.
—
Reply to this email directly, view it on GitHub
<#673 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJXDGBP25M7IGQW7OYL3GKTQZAVCNFSM6AAAAACAODDAF2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTAMRUGU2TONJWHA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
If it works for Sir @UmerShahidengr, otherwise I can show the my local changes to Sir when I meet him in person. |
That should work, thanks
…On Tue, Jul 1, 2025 at 9:10 AM sudo-apt-abdullah ***@***.***> wrote:
*sudo-apt-abdullah* left a comment (riscv-non-isa/riscv-arch-test#673)
<#673 (comment)>
If it works for Sir @UmerShahidengr <https://github.com/UmerShahidengr>,
otherwise I can show the my local changes to Sir when I meet him personally.
—
Reply to this email directly, view it on GitHub
<#673 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJQBIFQN5DPDTNTXEXT3GKXGNAVCNFSM6AAAAACAODDAF2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTAMRUGY3DKMBQHE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
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 |
d293d47
to
34b9b17
Compare
34b9b17
to
a64e25a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTG
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.