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

Added Zfinx and Zfh tests #367

Merged
merged 6 commits into from
Mar 26, 2024
Merged

Added Zfinx and Zfh tests #367

merged 6 commits into from
Mar 26, 2024

Conversation

anuani21
Copy link
Contributor

@anuani21 anuani21 commented Jul 3, 2023

Description

  • Zfh and Zfinx Test are added
  • Added half width and half word into the NAN function to accommodate Zfh, as the base for Zfh is F which is also falls under the value as word
  • Introduced two new flags (ZFINX and ZDINX) to aid the compilation of floating ops into integer regs

Related Issues

NA

Ratified/Unratified Extensions

[✓] Ratified

List Extensions

  • Zfinx
  • Zfh

Reference Model Used

[✓] SAIL

Mandatory Checklist:

Ran reports are placed here

https://gitlab.com/ptprasanna/actreports.git

@pawks
Copy link
Collaborator

pawks commented Jul 3, 2023

As already discussed, this PR can be merged only after the corresponding PR for ISAC has been merged.

This was referenced Jul 18, 2023
Closed
Closed
@davidharrishmc
Copy link
Contributor

I would use these tests on CORE-V Wally when they are available.

PR72 for ISAC has been merged.
riscv-software-src/riscv-isac#72

PR67 from CTG has been waiting for review since July 2.
riscv-software-src/riscv-ctg#67

@anuani21
Copy link
Contributor Author

anuani21 commented Jan 3, 2024

@neelgala , -Please have your review and let us know if more information would help.

@davidharrishmc
Copy link
Contributor

I see there are no fma tests in this directory. Is there a reason these are omitted?

@anuani21
Copy link
Contributor Author

anuani21 commented Jan 16, 2024 via email

@davidharrishmc
Copy link
Contributor

davidharrishmc commented Jan 16, 2024 via email

@davidharrishmc
Copy link
Contributor

davidharrishmc commented Jan 24, 2024 •
edited
Loading

On further review of the ratified Zfh spec, there are fmadd.h and related instructions. See the bottom figure of Section 15.2.

src3 H src2 src1 RM dest F[N]MADD/F[N]MSUB

https://wiki.riscv.org/display/HOME/Recently+Ratified+Extensions
https://drive.google.com/file/d/1z3tQQLm5ALsAD77PM0l0CHnapxWCeVzP/view

@allenjbaum
Copy link
Collaborator

allenjbaum commented Jan 24, 2024 via email

@anuani21
Copy link
Contributor Author

anuani21 commented Jan 25, 2024 via email

@davidharrishmc
Copy link
Contributor

davidharrishmc commented Jan 25, 2024 via email

@anuani21
Copy link
Contributor Author

anuani21 commented Jan 25, 2024 via email

@allenjbaum
Copy link
Collaborator

allenjbaum commented Jan 25, 2024 via email

allenjbaum
allenjbaum reviewed Jan 30, 2024
View reviewed changes
riscv-test-suite/env/arch_test.h
#else
#define SIGALIGN 4
#endif
#elif ZDINX==1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong; ZFinx only needs 4B registers, NOT 8B registers. And, if both Zfinx and Zdinx exist, then you need to accommodate both sizes, don't you? And, they're different on RV64 vs. RV32. So Zdinx would need to execute 2 separate LW ops on an RV32 compared to a single LD that RV64 to replace a FLD.

@davidharrishmc
Copy link
Contributor

CORE-V Wally will take advantage of the FMA tests as soon as you have them available.

@anuani21
Copy link
Contributor Author

anuani21 commented Feb 6, 2024

I have updated FMA tests for Zfh and Zfinx extension.

@davidharrishmc
Copy link
Contributor

I tried them out and they are working for me. The f{madd/msub/nmadd/nmsub}_b15-01 tests are not in the PR. These are the largest set of FMA tests. Is there a reason they aren't applicable to Zfh?

@anuani21
Copy link
Contributor Author

anuani21 commented Feb 6, 2024 via email

@anuani21
Copy link
Contributor Author

anuani21 commented Feb 7, 2024

@davidharrishmc, Are you facing any issue while running Zfinx test?

@davidharrishmc
Copy link
Contributor

I don't have hardware support for Zfinx, so I don't know if those tests have any issues.

@anuani21
Copy link
Contributor Author

anuani21 commented Feb 7, 2024 via email

@anuani21
Copy link
Contributor Author

anuani21 commented Feb 7, 2024

@davidharrishmc, I have updated f{madd/msub/nmadd/nmsub}_b15-01 tests in the PR.

davidharrishmc added a commit to davidharrishmc/cvw that referenced this pull request Feb 7, 2024
@davidharrishmc
Added ZFH FMA tests from riscv-non-isa/riscv-arch-test#367
5bde0db
@davidharrishmc
Copy link
Contributor

The Zfh fma _b15 tests run for CORE-V Wally.

@davidharrishmc
Copy link
Contributor

The fcvt.h.l tests should only depend on the F and Zfh extensions, as indicated in the test case.

RVTEST_CASE(0,"//check ISA:=regex(.*64.*I.*F.*Zfh.*);def TEST_CASE_1=True;",fcvt.h.l_b25)
inst_0:// rs1==x31, rd==f31,rs1_val == 0 and  fcsr == 0 and rm_val == 7  
/* opcode: fcvt.h.l ; op1:x31; dest:f31; op1val:0x0; valaddr_reg:x3;
val_offset:0*8; rmval:dyn; correctval:??; testreg:x2;
fcsr_val: 0*/
TEST_FPIO_OP(fcvt.h.l, f31, x31, dyn, 0, 0, x3, 0*8, x4, x1, x2,ld)

However, the TEST_FPIO_OP macro generates a fsd, which requires the D extension. Therefore, these tests do not run successfully on a RV64IFZfh machine.

0000000080000440 <inst_0>:
    80000440:	0001bf83          	ld	t6,0(gp)
    80000444:	00000113          	li	sp,0
    80000448:	00311073          	fscsr	sp
    8000044c:	d42fffd3          	fcvt.h.l	ft11,t6
    80000450:	00302273          	frcsr	tp
    80000454:	01f0b027          	fsd	ft11,0(ra)
    80000458:	0040b423          	sd	tp,8(ra)

It would be better to produce a fsw or fsh in this situation.

@davidharrishmc davidharrishmc mentioned this pull request Mar 15, 2024
Closed
@allenjbaum
Copy link
Collaborator

I see no further commits that would indicate that the problem David found (tests use D-ext ops on a configuration that only supports F-extension) have been fixed. Has that one been fixed?

As far as I can tell, that "fsd" corresponds to the first parameter of TEST_FPIO_OP ("inst" parameter )
but I can't find any instance of "fsd* used anywhere in the repo, so I don't have any idea where it is coming from.

@allenjbaum
Copy link
Collaborator

Actually, that "fsd" op corresponds to something after the FPIO macro, but all the FP tests that use the FPIO macro consist of nothing but FPIO macros. the only ops that use FPIO are FCVT ops, and there are no uses of fsd in them, therefore it must be after the FPIO. But, all of those tests end with no further ops. Search for "fsd" shows only tests of fsd, and the don't use FPIO. So, I cannot figure out where that fsd is being generated.

@anuani21
Copy link
Contributor Author

anuani21 commented Mar 24, 2024 via email

@anuani21
Copy link
Contributor Author

@davidharrishmc and @allenjbaum,

I have updated the test using fsw instead of fsd. I have attached the logs for your reference for fcvt.h.l instruction.

00000000800003ac <inst_0>:
800003ac: 0001bf83 ld t6,0(gp)
800003b0: 00000113 li sp,0
800003b4: 00311073 fscsr sp
800003b8: d42fffd3 fcvt.h.l ft11,t6
800003bc: 00302273 frcsr tp
800003c0: 01f0a027 fsw ft11,0(ra)
800003c4: 0040b223 sd tp,4(ra)

@davidharrishmc
Copy link
Contributor

This fix to RVTEST_ISA looks good.

There is still an inconsistency between the check ISA lines in fcvt.h.l_b25 and _b26. The instruction should depend on RV64, so I think the _b25 line should add *RV64 to match _b26.

RVTEST_CASE(0,"//check ISA:=regex(.*I.*F.Zfh.);def TEST_CASE_1=True;",fcvt.h.l_b25)
RVTEST_CASE(0,"//check ISA:=regex(.*RV64.*I.*F.Zfh.);def TEST_CASE_1=True;",fcvt.h.l_b26)

The same applies to fcvt.h.lu_b25.

The check ISA is also wrong (usually has D, always missing RV64) on fcvt.l.h*, fcvt.lu.h*

Nevertheless, the incorrect macros are not preventing me from building and running the tests. The new tests pass on CORE-V Wally.

@allenjbaum
Copy link
Collaborator

allenjbaum commented Mar 25, 2024 via email

@davidharrishmc
Copy link
Contributor

davidharrishmc commented Mar 25, 2024 via email

@allenjbaum
Copy link
Collaborator

Oh, and anuani21 - thanks for figuring out what the problem is.
Now we know we should carefully review this macros as well as the tests.
It is still a puzzle to me that this wasn't caught ages ago.

allenjbaum
allenjbaum previously requested changes Mar 25, 2024
View reviewed changes
Copy link
Collaborator

@allenjbaum allenjbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, and thanks for figuring out what the problem was. I would ask that you back out the changes in the comments however, as the clutter up the diff. We can merge it as soon as that is done.

@allenjbaum
Copy link
Collaborator

allenjbaum commented Mar 25, 2024 via email

@allenjbaum allenjbaum dismissed their stale review March 25, 2024 22:01

on further reflection, the changes I asked for aren't necessary

allenjbaum
allenjbaum approved these changes Mar 25, 2024
View reviewed changes
Copy link
Collaborator

@allenjbaum allenjbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unneccessarily (in my opinion, anyway) edits comments - but I don't think that will cause future problem. It also has a simple fix for the issues that were seen. We may want further tests in the future to more carefully look at corner cases for better coverage, but this is at a minimum a good start

@allenjbaum
Copy link
Collaborator

sigh: as usual, we have a conflict with the changelog version. It needs to be updated.
Note that in future, all PRs should be filed against the dev branch, the dev branch should not be checking for changelog version numbers, and maintainers will be responsible for updating the union of changelogs for all the PRs included in that version of the dev branch

@anuani21
Copy link
Contributor Author

@allenjbaum, I have updated the changelog file. Please have your review and let us know if more information would help.

Hereafter, I will raise the PR with target branch as dev.

allenjbaum
allenjbaum approved these changes Mar 26, 2024
View reviewed changes
Copy link
Collaborator

@allenjbaum allenjbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the problem, and the previous fixes the earlier problem of specifying D support for an extension that doesn't need it.

@allenjbaum allenjbaum merged commit 7553caa into riscv-non-isa:main Mar 26, 2024
1 check passed
@stnolting
Copy link
Contributor

Just one question about this: Zfinx is currently only supported for rv64 machines, right?! 🤔

(there is a rv64i_m/Zfinx testcases folder but no rv32i_m/Zfinx testcases folder)

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

@allenjbaum allenjbaum allenjbaum approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@anuani21 @pawks @davidharrishmc @allenjbaum @stnolting