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

[davidharrishmc]

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

• 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
• Spike
• Other - < SPECIFY HERE >

Mandatory Checklist:

• All tests are compliant with the test-format spec present in this repo ?
• Ran the new tests on RISCOF with SAIL/Spike as reference model successfully ?
• Ran the new tests on RISCOF in coverage mode
• Link to Google-Drive folder containing the new coverage reports (See this for more info): < SPECIFY HERE >
• Link to PR in RISCV-ISAC from which the reports were generated : < SPECIFY HERE >
• 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]

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.

e3c5471#r143001732

[Timmmm]

Thanks!

I have a PR to add cbo.zero support to Sail here if you want to test it:

riscv/sail-riscv#455

However I'd say it's not necessary for you to test it - I made the same changes locally and it selects the test now & passes. (I would have made a PR but you were faster than our PR approval process!)

[UmerShahidengr]

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

[Timmmm]

Thanks!

[allenjbaum]

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 <#473 (comment)> : 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 <#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 <#473 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJWSMQ4MK7YZGX6ONA3ZHAP5DAVCNFSM6AAAAABJE7T3W6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMJSGYZDCOBSHE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[jamesbeyond]

LGTM