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.

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

Zcf/Zcd RVTEST_CASE macros inconsistent #590

Closed
jordancarlin opened this issue Dec 31, 2024 · 5 comments · Fixed by #597
Closed

Zcf/Zcd RVTEST_CASE macros inconsistent #590

jordancarlin opened this issue Dec 31, 2024 · 5 comments · Fixed by #597

Comments

@jordancarlin
Copy link
Contributor

The new Zcf and Zcd tests introduced in #497 and #587 are not consistent in their use of the RVTEST_CASE isa regex macro.

For rv32 and rv64 Zcd, c.fsdsp uses check ISA:=regex(.*I.*D.*C.*) while all of the other tests use check ISA:=regex(.*I.*F.*D.*C.*). D implies F so this one isn't a big deal, but they should be consistent.

More significantly, for rv32 Zcf, c.fswsp uses check ISA:=regex(.*I.*F.*C.*) while all of the other tests use check ISA:=regex(.*I.*F.*Zcf.*). Either approach is acceptable, but using Zcf requires modifying the ISA string that is passed to RISCOF when selecting tests.

All of the Zcf and Zcd tests should use the same approach for this, either using the Zcf and Zcd extensions or the C and F/D extensions.

@jordancarlin
Copy link
Contributor Author

Related and even more problematic, the c.fldsp test for rv32i_m specifies rv64 instead of rv32 (RVTEST_ISA("RV64IFDC"))

@jordancarlin
Copy link
Contributor Author

The RVTEST_CASE check ISA regex is also missing 32/64 for rv32 and rv64 respectively.

@jordancarlin
Copy link
Contributor Author

@anuani21 @UmerShahidengr Is there a preference for one approach to the ISA string over another? I'm happy to open a PR to fix this, but we need to decide if we want that ISA string to use C or Zcf/Zcd.

@UmerShahidengr
Copy link
Collaborator

Good catch @jordancarlin , I didnt check the ISA string for all tests, I think it should be consistent with Zcf/Zcd strings instead of C as these tests belong to the respective Zcf and Zcd extensions and not all C-extension enabled designs supports these tests

@UmerShahidengr
Copy link
Collaborator

Would be great if you open up a PR to resolve this issue, we can do one thing, I can merge Zhix tests too, and @anuani21 and @jordancarlin can make all tests consistent via same PR. I am seeing some Zhinx tests also have the same issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants