search

riscv-non-isa / riscv-arch-test

https://jira.riscv.org/browse/RVG-141?src=confmacro
Apache License 2.0
503 stars 191 forks source link

Fixed reversed order of Zicboz and Zicsr in macros for cbo.zero #473

Closed davidharrishmc closed 3 months ago

davidharrishmc commented 3 months ago
## Description > Provide a detailed description of the changes performed by the PR. ### Related Issues > Please list all the issues related to this PR. Use NA if no issues exist ### Ratified/Unratified Extensions - [x] Ratified - [ ] Unratified ### List Extensions > List the extensions that your PR affects. In case of unratified extensions, please provide a link to the spec draft that was referred to make this PR. ### Reference Model Used - [ ] SAIL - [x] Spike - [ ] Other - < SPECIFY HERE > ### Mandatory Checklist: - [ ] All tests are compliant with the test-format spec present in this repo ? - [x] Ran the new tests on RISCOF with SAIL/Spike as reference model successfully ? - [ ] Ran the new tests on RISCOF in [coverage mode](https://riscof.readthedocs.io/en/stable/commands.html#coverage) - [ ] Link to Google-Drive folder containing the new coverage reports ([See this](https://github.com/riscv-non-isa/riscv-arch-test/blob/main/CONTRIBUTION.md#uploading-test-stats) for more info): < SPECIFY HERE > - [ ] Link to PR in RISCV-ISAC from which the reports were generated : < SPECIFY HERE > - [x] Changelog entry created with a minor patch ### Optional Checklist: - [ ] RISCV-V CTG PR link if tests were generated using it : < SPECIFY HERE > - [ ] Were the tests hand-written/modified ? - [ ] Have you run these on any hard DUT model ? Please specify name and provide link if possible in the description - [ ] If you have modified arch\_test.h Please provide a detailed description of the changes in the Description section above.
davidharrishmc commented 3 months ago

Note that Sail does not yet support cbo.zero, so Sail throws an illegal instruction exception when attempting to simulate it during riscof compilation. Consequently I have been unable to fully test this. However, it should fix the problem Tim Hutt raised about the commit.

https://github.com/riscv-non-isa/riscv-arch-test/commit/e3c5471f58573ea99cd282197251e7c453ba1696#r143001732

UmerShahidengr commented 3 months ago

@jamesbeyond please have a look at it, this one is good to go, so you may merge this one

allenjbaum commented 3 months ago

Regarding " It appears that the cbo tests don't use CSRs and that the Zicsr would likely be unnecessary. " While CBO tsts don't explicitly use CSRs, if there is any possibility they would trap, they can implicitly use them (at least, for arch-tests), So Zicsr can be removed if there is no possibility of a trap.

On Wed, Jun 12, 2024 at 3:34 AM David Harris @.***> wrote:

@.**** commented on this pull request.

On riscv-test-suite/rv64i_m/CMO/src/cbo.zero-01.S https://github.com/riscv-non-isa/riscv-arch-test/pull/473#discussion_r1636222460 :

I added Zicsr to the RV64 cbo.zero RVTEST_ISA string for consistency with the other tests per @UmerShahidengr https://github.com/UmerShahidengr 's comment.

I'm not the author of cbo. It appears that the cbo tests don't use CSRs and that the Zicsr would likely be unnecessary. I believe somebody from my team had experimented with that months ago and presumably removed the Zicsr in just this case, but I agree that it is better to be consistent. It's hard to envision a system with caches but no CSRs, so Zicsr is likely harmless, and if the tests are ever generalized to non-machine mode, Zicsr will be nessary to support menvcfg and the other modes.

@liweiwei90 https://github.com/liweiwei90 was the original author of PR

226 https://github.com/riscv-non-isa/riscv-arch-test/pull/226 with

cbo.zero tests. I will defer to the author if they decide to remove Zicsr from cbo.zero at a later point. If so, I think it should come out of RVTEST_ISA and RVTEST_CASE for both rv32i_m and rv64i_m.

— Reply to this email directly, view it on GitHub https://github.com/riscv-non-isa/riscv-arch-test/pull/473#pullrequestreview-2112621829, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHPXVJWSMQ4MK7YZGX6ONA3ZHAP5DAVCNFSM6AAAAABJE7T3W6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMJSGYZDCOBSHE . You are receiving this because you are subscribed to this thread.Message ID: @.***>