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.

Sign up for GitHub

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

Jump to bottom

Priv 1.12 ACT - Miscellaneous Missing Tests and Coverage additions #26

Closed
13 tasks
jjscheel opened this issue Mar 17, 2023 · 49 comments
Closed
13 tasks

Priv 1.12 ACT - Miscellaneous Missing Tests and Coverage additions #26

jjscheel opened this issue Mar 17, 2023 · 49 comments
Assignees
@jjscheel @allenjbaum @UmerShahidengr

Comments

@jjscheel
Copy link
Contributor

jjscheel commented Mar 17, 2023 •
edited
Loading

Technical Group

Architecture Test SIG

ratification-pkg

Priv 1.11, Priv 1.12

Technical Liaison

Allen Baum

Task Category

Arch Tests
65

Task Sub Category

  • gcc
  • binutils
  • gdb
  • intrinsics
  • Java
  • KVM
  • ld
  • llvm
  • Linux kernel
  • QEMU
  • Spike

Ratification Target

2021

Statement of Work (SOW)

SOW: link

SOW Signoffs:

  • Task group liaison sign-off date:
  • Development partner sign-off date: Umer (Oct 10, 2023)
  • ACT SIG sign-off date (if ACT work):

Waiver

  • Freeze
  • Ratification

Pull Request Details
@jjscheel jjscheel self-assigned this Mar 17, 2023
@jrtc27
Copy link

jrtc27 commented Mar 17, 2023

I don’t think that’s the right SOW?

@jjscheel
Copy link
Contributor Author

@jrtc27, that's the one I intended (and that I was given), but it is indeed new and it could be that our title doesn't make sense. Look for updates after our next meeting where will indeed be discussing.

THANKS!

@jjscheel
Copy link
Contributor Author

@UmerShahidengr, can you review this item in the context of @jrtc27's question. I'm pretty sure I've linked the SOW you sent me. Perhaps my title is too ambiguous.

@jjscheel
Copy link
Contributor Author

Actually, at reviewing things more, I think this is a duplicate of #13. I'm going to close it for now.

@jjscheel jjscheel closed this as not planned Won't fix, can't repro, duplicate, stale Mar 17, 2023
@allenjbaum
Copy link

allenjbaum commented Mar 17, 2023 via email

@jjscheel
Copy link
Contributor Author

Ok, let's re-open and continue looking at this next meeting.

@jjscheel jjscheel reopened this Mar 17, 2023
@jrtc27
Copy link

jrtc27 commented Mar 17, 2023

The SOW has the following requirements:

Generate tests for M-extension
Divide/Remainder tests (issue#300)
Specifically tests of the most negative integer divided by -1.
This should include all forms of signed DIV and REM only
Add better test generator coverage conditions and regenerate non-FP op tests (issue #306)
rs1 == rd != rs2; rd != x0
rs1 == rd != rs2; rd == x0
rs1 == x0 != rd
rd == x0 != rs1
Generate tests that use unsupported, legal encodings for Fence and Fencei ops (issue#119)
Specifically that any reserved bits defined to be ignored do not cause traps.
Generate tests for c.flw and c.fsw for RV64 (issue#289) currently missing entirely
Delete #ifdef rvtest_mtrap_routine, and matching #endif lines from all tests (issue #292)
Remove TEST_RRI_OP macro from test_macros.h

None of those are part of the privileged spec.

@allenjbaum
Copy link

The SOW is correct, and is different from the #13; the title of this shouldn't be "legacy corner case" IMHO, but rather
"Miscellaneous Missing Tests and Coverage additions"

@allenjbaum
Copy link

allenjbaum commented Mar 17, 2023 via email

@jjscheel jjscheel changed the title Priv 1.12 ACT - Legacy "corner cases" Priv 1.12 ACT - Miscellaneous Missing Tests and Coverage additions Mar 19, 2023
@jjscheel
Copy link
Contributor Author

Thanks, @allenjbaum and @jrtc27

@jjscheel
Copy link
Contributor Author

@UmerShahidengr, another item needing an update:

  • Who will be working on this item?
  • When do we envision them starting?
  • If they've already started, what is the current status?

Thanks!

@UmerShahidengr
Copy link

Our trainees (which are recruited last month) will be working on this project. Each trainee will work on one issue as part of the training. I will assign one trainee from the current batch to be the collaborator, then this SoW can be assigned to him. This SoW has not been started yet but it will be completed in 2023Q2.

@jjscheel
Copy link
Contributor Author

Thanks, I will set projected date to end of 2Q and mark it for discussion in next meeting.

@UmerShahidengr
Copy link

Update May 7th, 2023 ⇾ Not much update on this task yet.

@jjscheel
Copy link
Contributor Author

jjscheel commented May 8, 2023

Ok. Thanks. Setting next update to next meeting and dropping from this Agenda.

@UmerShahidengr
Copy link

Update May 23rd, 2023 => Not much update on this task yet.

@UmerShahidengr
Copy link

Update ⇾ June 12th, 2023:
The tasks have been distributed among the trainees, hopefully it will be completed within next 2,3 weeks

@jjscheel
Copy link
Contributor Author

Thanks, @UmerShahidengr. I've removed from agenda.

@UmerShahidengr
Copy link

Update:
There were 4 issues to be resolved in this SoW.

  1. Issue#300: This issue has been resolved. Its PR is riscv-arch-test is here, whereas the coverage points added for riscv-ctg is given in this PR at riscv-ctg
  2. Issue#306: This issue has been assigned to @alitariq4589 and will be resolved soon.
  3. issue#289: According to my understanding, compressed floating point coverage is the part of Zc* extension support and that SoW has already been assigned to Prassanna. This issue is asking to add coverage support for c.flw and c.fsw, but there is no support for any compressed floating point instruction so full Zce coverage is required on which Prassanna is already working on, so this issue can be merged with Prassanna's SoW.
  4. issue#292: This issue has already been closed.

@UmerShahidengr
Copy link

After transferring Issue#289 to Prassanna, and resolving Issue#306, this SoW will be completed.

@ptprasanna
Copy link

@ptprasanna, if we can get agreement that issue Arch Test issue #289 will be driven by IITM, I believe we can close this. Would you have a look at that?

@jjscheel Yes, as part of Zcc we are covering up Zcf and Zcd (Compressed instruction of floating single and double)

@jjscheel
Copy link
Contributor Author

So, @UmerShahidengr, if I read all the issues correctly, only the first has a PR and while it's been submitted, it has yet to be accepted.

If correct, we need 2 things to occur before we can close this out:

  1. The PR needs to be accepted.
  2. The SOW submitter (Allen) needs to accept that the work is complete.

@alitariq4589
Copy link

@jjscheel Issue 300 and Issue 306 both have PRs in place, there is just some mandatory checklist which needs to be completed. I will get those all sorted out by next week hopefully. And yes your two points are correct.

@jjscheel
Copy link
Contributor Author

Thanks, @alitariq4589! I've updated the summary in the first comment.

@UmerShahidengr
Copy link

Update ⇾ Aug 15th, 2023
Issue#306 is stalled because of the limitations in CTG, more details are in this comment.
This CTG issue will be resolved soon by @alitariq4589, then Issue#306 will be resolved.

@UmerShahidengr
Copy link

Issue#300 has been resolved. Its PR has been merged.
The only thing left now is Issue#306.

@jjscheel FYI

@UmerShahidengr
Copy link

Update ⇾ September 12th, 2023
Issue#306 has also been resolved. Its PR has been submitted in riscv-ctg here

@jjscheel
Copy link
Contributor Author

@UmerShahidengr, this sounds like positive status. Does this mean all work has been submitted as PRs and all PRs are accepted?

@UmerShahidengr
Copy link

All work has been submitted. The most recent PR hasn't been meeged yet.
Rest of the work has been merged.

@UmerShahidengr
Copy link

Update Sept 26th, 2023:
The CTG PR has also been merged.

@UmerShahidengr
Copy link

Update ⇾ October 10th, 2023:
All PRs related to this SoW have been merged, so this can be closed now.

@jjscheel
Copy link
Contributor Author

All PRs merged means we just have to sign-off on SOW. @allenjbaum, can you please confirm that all is complete from your perspective as Task contact and ACT contact?

@allenjbaum
Copy link

I tried to approve the merge, and got 318 conflicts. There were two separate versions of these tests.
sample: <<<<<<< alitariq4589/Issue#306
// version : 0.11.0
// timestamp : Tue Jul 4 11:47:27 2023 GMT
// usage : riscv_ctg
// -- cgf // --cgf /home/ali/riscv-ctg-alitariq4589/riscv-ctg/sample_cgfs/dataset.cgf
// --cgf /home/ali/RISCOF/issue306.cgf
\

// version : 0.11.1
// timestamp : Mon Sep 18 19:38:51 2023 GMT
// usage : riscv_ctg
// -- cgf // --cgf /home/hammad/Ali_Tariq_solution/riscv-ctg/sample_cgfs/dataset.cgf
// --cgf /home/hammad/Ali_Tariq_solut

@allenjbaum
Copy link

allenjbaum commented Oct 11, 2023 via email

@alitariq4589
Copy link

There's a merge conflict on PR#368; it looks like something needed to be rebased before pushing iit.
…
On Tue, Oct 10, 2023 at 5:21 AM Jeff Scheel @.> wrote: All PRs merged means we just have to sign-off on SOW. @allenjbaum https://github.com/allenjbaum, can you please confirm that all is complete from your perspective as Task contact and ACT contact? — Reply to this email directly, view it on GitHub <#26 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHPXVJRKPWRCKHF36UVXHSDX6U4TZAVCNFSM6AAAAAAV6VME2WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJVGI3DSOBYGM . You are receiving this because you were mentioned.Message ID: @.>

@allenjbaum The issue#306 of arch-test needed some changes in the source code of riscv-ctg. So, the initially created PR#368 (which was created by me) did not generate all cases and I forgot to close the PR#368. In PR#385, MuhammadHammad001 changed the source code of RISC-V CTG and generated all required assembly tests resolving the issue#306. So I will close the PR#368 now.

@allenjbaum
Copy link

I'm quite confused as to which PRs were merged as part of this
but trolling through the history, it says that issue 300 (dividing most negative by -1)was solved by pull#365.
But pull 365 affects only divw and remw - but not div or rem.
Moreover, eyeballing Div01.S I see no tests that divide by -1 at all!
I think the list of constants need to include both 0 and -1 (and largest negative/positive values),
as well as their cross products

These test cases are fixed in div.s :
rs1 == rd != rs2; rd != x0
rs1 == rd != rs2; rd == x0
rs1 == x0 != rd
rd == x0 != rs1
But, they probably need to also be in divw.s (and maybe rem and rem,w)
not to mention divu, divu.w, & maybe remu.w

The SOW says that the TEST_RRI_OP macro should be removed from test.h; it hasn't been
IT SOW say  Delete #ifdef rvtest_mtrap_routine, and matching #endif lines from all tests (issue #292)
I thought that had been done at one point, but I still see them there.

So, this doesn't look complete at all. Am I missing something here? or is there some reason those were removed from the SOW?

@alitariq4589
Copy link

@allenjbaum You got that right regarding rem and div that the test condition is not there. But I added the condition in rfmt_val_comb_sgn inside dataset.cgf and rfmt_val_comb_sgn is responsible for generating tests with added conditions in both kind of instructions (divw/remw and div/rem). But it seems to me while I was generating the tests, I misunderstood the requirement and generated only for remw and divw and not for rem and div (which can also be seen by the title of PR#365). The support for this is already added in riscv-ctg by merged PR#65 (as both type of instructions share the same label) so it is just a matter of generating the tests for rem and div.

I will generate the tests and will create another PR for rem/div for the case division of most negative integer divided by -1.

As far as issue#292 and TEST_RPI_OP labels are concerned, issue#292 was closed on Mar 27, 2023 (as completed) so the label should have been removed from the PR because I did not add it explicitly and the latest commit in the branch from which I created PR was of May 8, 2023 which implies that the label should not have been there. But I will see to it that it is removed when I create PR for rem and div

@UmerShahidengr
Copy link

Let me explain this whole scenerio, there were two issues to be solved as part of this SoW:

  1. Issue#300:
  • It required some updates in CTG which were merged here.
  • The generated tests were merged here. The PR#365 only had divw and remw tests on the updated conditions for the proof of concept, div and rem tests were not changed as coverpoint of dividing by -1 was hit for division and reminder operations by divw and remw.
  1. Issue#306:
  • This issue also required modifications in CTG, changes were merged here.
  • The generated tests based on the updated conditions were merged here.

Issue#292 was already closed when we started working on it, so nobody worked on it from our side.

@allenjbaum
Copy link

allenjbaum commented Oct 12, 2023 via email

@UmerShahidengr
Copy link

Update ⇾ October 24th, 2023
No update on this.

@alitariq4589
Copy link

I have added the PR in riscv-arch-test for solving issue#300 (most negative number divided by -1). This PR contains tests for both RV32IM as well as RV64IM for rem* and div*. The relevant riscv-ctg PR is also mentioned in the checklist.

@UmerShahidengr
Copy link

Update ⇾ November 28th, 2023
The PR related to Issue#300 has been merged, now there is no outstanding work left, so this SoW can be closed.

@jjscheel
Copy link
Contributor Author

@allenjbaum, please review and confirm that we can consider this done. Thanks!

@allenjbaum
Copy link

If it's been merged, it can be considered done.

@UmerShahidengr
Copy link

Update ⇾ December 12th, 2023
You can close this one

@jjscheel
Copy link
Contributor Author

Closed. Thanks to all who completed this work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jjscheel jjscheel

@allenjbaum allenjbaum

@UmerShahidengr UmerShahidengr

Labels
None yet
Projects
RISC-V DevPartner Work
Status: As-planned
Milestone
No milestone
Development

No branches or pull requests
6 participants
@jrtc27 @jjscheel @allenjbaum @UmerShahidengr @ptprasanna @alitariq4589