-
Notifications
You must be signed in to change notification settings - Fork 232
Fixed Coverage Holes for RV32F #670
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 RV32F #670
Conversation
Yes, I worked to bring the tests up to 100% coverage, which was initially very low because the val_comb coverpoints weren’t being hit due to incorrect dataset values. At first, I regenerated the tests with a corrected dataset. However, in the latest commit, I simply fixed the dataset using a Python script, since the test cases themselves were already correct. |
The reason I asked is that all the tests were completely replaced, and if
it were just the coverpoints being updated, then I wouldn't have expected
any changes to the .S files,
Can you make it a bit more clear why the tests themselves were regenerated,
since the test cases were already correct?
…On Sat, Jun 28, 2025 at 4:35 PM sudo-apt-abdullah ***@***.***> wrote:
*sudo-apt-abdullah* left a comment (riscv-non-isa/riscv-arch-test#670)
<#670 (comment)>
Yes, I worked to bring the tests up to 100% coverage, which was initially
very low because the val_comb coverpoints weren’t being hit due to
incorrect dataset values.
At first, I regenerated the tests with a corrected dataset. However, in
the latest commit, I simply fixed the dataset using a Python script, since
the test cases themselves were already correct.
—
Reply to this email directly, view it on GitHub
<#670 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJUMOWU5ZY7VP53QJN33F4RD7AVCNFSM6AAAAACAJCMXLCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTAMJWGE2DSMZWGM>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Thank you for pointing that out, and I apologize for the confusion caused by GitHub showing the entire file as changed. To clarify, the test cases themselves were not regenerated — they remain exactly the same. The only modification was at the end of each file, where the dataset values are defined. These were updated using a Python script to ensure that the val_comb coverpoints are now correctly hit. The original test logic was valid; the issue was solely with incorrect operand values in the dataset section, which led to poor coverage initially. Unfortunately, due to line-ending normalization or whitespace changes (possibly introduced by tooling or script formatting), GitHub's diff view shows the entire file as changed, even though the actual update is limited to the dataset portion. You can confirm this by comparing the beginning and middle sections of each file — only the dataset at the end has been updated. |
…into F_coverage
54160e3
to
43dae9a
Compare
I have checked the test files 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. |
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.
LGTM
The coverage was initially dropped due to the incorrect operand values. Each test has an associated dataset at its end, but the dataset values were incorrect.
op1val: 0x7f800001
op2val: 0xff800000
But the dataset incorrectly records:
NAN_BOXED(0x7f800001139095041,32,FLEN)
NAN_BOXED(0xff800000286578688,32,FLEN)
To correct this and achieve full coverage, I regenerated the tests with the correct boxing logic. Here's the difference in between the initial and final coverage can be seen: https://docs.google.com/spreadsheets/d/1V1TRYfl7aYsMEXnGEAn7FwO558mKAwf9a3cCnG6dBow/edit?gid=0#gid=0