Add support for the Zfa ISA extension #330

cmuellner · 2023-04-06T12:29:07Z

This patch introduces the RISC-V Zfa extension, which introduces additional floating-point extensions:

fli (load-immediate) with pre-defined immediates
fminm/fmaxm (like fmin/fmax but with different NaN behaviour)
fround/froundmx (round to integer)
fcvtmod.w.d (Modular Convert-to-Integer)
fmv* to access high bits of float register bigger than XLEN
Quiet comparison instructions (fleq/fltq)

Zfa defines its instructions in combination with the following extensions:

single-precision floating-point (F)
double-precision floating-point (D)
quad-precision floating-point (Q)
half-precision floating-point (Zfh)

Since the RISC-V architecture test framework does not support the RISC-V quad-precision floating-point ISA extension (Q) and the RISC-V half-precision floating-point ISA extension (Zfh), this patch does not include tests for instructions that depend on these extensions.

Given missing infrastructure support, the fli.* instruction tests are hand written and not generated.
All other test files are generated using riscv-ctg.

The generated test files have been generated using the following command (with $BASEISA={rv32i,rv64i}, $FLEN={32,64}), and CFG=$INSN:
riscv_ctg.py
--cgf sample_cgfs/dataset.cgf
--cgf sample_cgfs/zfa/$INSN.cfg
--base-isa $BASEISA
--flen $FLEN
-d tests_$BASEISA_$FLEN/

Exceptions are:

fcvtmod.w.d.cgf is for FLEN=64 only
fmvh.x.d and fmvp.d.x are for BASEISA=rv32i only

The resulting tests have been copied to the target directory.

The generation of the Zfa test cases depends on a PR in the riscv-ctg repository:
riscv-software-src/riscv-ctg#60

The Zfa specification is ratified and can be found here:
https://github.com/riscv/riscv-isa-manual/blob/main/src/zfa.adoc

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.

charmitro · 2023-04-07T22:40:34Z

Link for test results using RISCOF can be found here.

pawks · 2023-04-10T06:27:44Z

This PR also lacks the coverage reports and the CGF files. Please include them in the drive folder.

cmuellner · 2023-04-11T09:12:57Z

The CGF files are in the linked riscv-ctg PR.

The coverage report is blocked by #331.

cmuellner · 2023-05-10T22:56:26Z

Updated the PR: fixed the rounding modes for fcvtmod.w.d, which must be rtz (all other modes are reserved).

cmuellner · 2023-05-23T09:59:48Z

Updated PR:

Rebased and updated version
Remove stale fsqrt test cases
The previous fixup of the fcvtmod instructions was not sufficient. Since the tests are generated, the generator file has been adjusted accordingly in Add support for the Zfa ISA extension riscv-software-src/riscv-ctg#60.
From that all test cases have been regenerated.

jscheid-ventana · 2023-11-17T01:11:29Z

Now that Zfa is ratified (https://wiki.riscv.org/display/HOME/Specification+Status#SpecificationStatus-Zfa_Row) what is next?

davidharrishmc · 2024-01-13T02:46:05Z

The fcvtmod tests need to replace dynamic with rtz. See the riscv-zfa spec at Section 264:

"The assembly syntax requires the RTZ rounding mode to be explicitly specified, i.e., fcvtmod.w.d rd, rs1, rtz"

Looks like this is mentioned above, but I don't see the rtz in the file.

davidharrishmc · 2024-01-13T14:51:31Z

Also, note that one of the tests (in both rv32i_m and rv64i_m) produces a discrepancy between Sail and Spike.

ERROR | /home/harris/cvw/addins/riscv-arch-test/riscv-test-suite/rv32i_m/D_Zfa/src/fcvtmod.w.d_b22-01.S : - : Failed

I believe Sail is setting the wrong flag. I have submitted the issue to
riscv/sail-riscv#388

davidharrishmc · 2024-01-16T19:47:23Z

I don't see froundnx tests in the repository even though they are in the ctg.

davidharrishmc · 2024-01-16T20:02:32Z

Why are there more fleq.s tests in D_Zfa/src/fleq_b1-01.S than in F_Zfa/src/fleq_b1-01.S? Why doesn't D_Zfa just add double-precision tests that aren't in F_Zfa?

davidharrishmc · 2024-01-16T20:06:01Z

The patch documentation above says that fcvtmod.w.d is for FLEN=32 only. I think that is a typo and should say for FLEN=64 only.

cmuellner · 2024-01-16T23:07:12Z

When attempting to regenerate the tests, I found riscv-ctg broken (triggering a code-path that always fires an exception). I've addressed this in riscv-software-src/riscv-ctg#102.

The fcvtmod tests need to replace dynamic with rtz. See the riscv-zfa spec at Section 264:

"The assembly syntax requires the RTZ rounding mode to be explicitly specified, i.e., fcvtmod.w.d rd, rs1, rtz"

Looks like this is mentioned above, but I don't see the rtz in the file.

fter looking into riscv-ctg, it becomes obvious why the 'dyn' rounding mode is still in this PR:
The Zfa riscv-ctg PR restricts the rounding modes as follows:

  rm_val_data: '[1]'

However, the field rm_val_data, which was expected to provide a mechanism to restrict the rounding mode, is completely ignored because the only referencing code has been removed in riscv-software-src/riscv-ctg@1d108e7.

So, this feature was dropped, and I can't see a replacement in CTG atm.

Also, note that one of the tests (in both rv32i_m and rv64i_m) produces a discrepancy between Sail and Spike.

ERROR | /home/harris/cvw/addins/riscv-arch-test/riscv-test-suite/rv32i_m/D_Zfa/src/fcvtmod.w.d_b22-01.S : - : Failed

I believe Sail is setting the wrong flag. I have submitted the issue to riscv/sail-riscv#388

Thanks for reporting!

I don't see froundnx tests in the repository even though they are in the ctg.

Thanks! This was caused by a typo in the CTG PR, which has been addressed.

Why are there more fleq.s tests in D_Zfa/src/fleq_b1-01.S than in F_Zfa/src/fleq_b1-01.S? Why doesn't D_Zfa just add double-precision tests that aren't in F_Zfa?

I don't see anything odd in the CTG PR so that it might be the intended behavior of CTG.

The patch documentation above says that fcvtmod.w.d is for FLEN=32 only. I think that is a typo and should say for FLEN=64 only.

Thanks! I've updated the description accordingly.

Thanks for reviewing the PR!
I've rebased and updated it as mentioned above.

davidharrishmc · 2024-01-17T05:24:32Z

Thanks for looking into all of these. I'm trying to test CORE-V Wally Zfa and discovering things as I go.
rv32i_m/D_Zfa is missing fmvp.d.x tests.

The regular D tests are exclusive of the cases already covered by F tests. For example, rv32i_m/D contains fadd.d but not fadd.s. By the same reasoning, it would seem that rv32i_m/D_Zfa should contain fltq.d but not fltq. Similarly, I don't think it needs to have fleq, fmaxm, fminm, or fround.

davidharrishmc · 2024-02-05T14:38:53Z

The CORE-V Wally team would love to try the remaining tests when possible. We're happy to help if there's something we can do.

davidharrishmc · 2024-03-06T20:51:27Z

Is it easy to add the missing fmvp.d.x tests?

rv32i_m/D_Zfa/fmvp.d.x

cmuellner · 2024-03-06T23:14:40Z

Is it easy to add the missing fmvp.d.x tests?

rv32i_m/D_Zfa/fmvp.d.x

I just checked and there are two issues why this test does not exist:

There is a typo in the riscv-ctg PR (riscv_ctg/data/fd.yaml): s/fmvh.d.x/fmvp.d.x
After fixing the typo the test generation fails, because riscv-isac can't handle that instruction

I have not analyzed this further so far.

davidharrishmc · 2024-03-25T23:30:39Z

Something happened to fcvtmod.w.d when the tests were regenerated. The rounding modes were changed from rz to dyn. This is incorrect - the instruction should always use rz per the spec: "It is encoded like FCVT.W.D, but with the rs2 field set to 8 and the rm field set to 1 (RTZ). Other rm values are reserved."

Here is the May 22, 2023 version of fcvtmod.w.d_b22 using rz.

inst_0:// rs1==f31, rd==x31,fs1 == 0 and fe1 == 0x3fc and fm1 == 0x08577924770d3 and fcsr == 0x0 and rm_val == 7
/* opcode: fcvtmod.w.d ; op1:f31; dest:x31; op1val:0x3fc08577924770d3; valaddr_reg:x3;
val_offset:0FLEN/8; rmval:rtz; correctval:??; testreg:x2;
fcsr_val:0/
TEST_FPID_OP(fcvtmod.w.d, x31, f31, rtz, 0, 0, x3, 0*FLEN/8, x4, x1, x2,FLREG)

The version currently in the PR has dyn instead of rz.

Could you regenerate the fcvtmod.w.d tests with rz?

allenjbaum · 2024-03-27T06:57:58Z

This is a bit weird. Presumably the test on this latest was run and passed, yet the instructions as defined should not have even assembled without errors; I didn't think the toolchain would allow it if the spec required rtz

This patch introduces the RISC-V Zfa extension, which introduces additional floating-point extensions: * fli (load-immediate) with pre-defined immediates * fminm/fmaxm (like fmin/fmax but with different NaN behaviour) * fround/froundmx (round to integer) * fcvtmod.w.d (Modular Convert-to-Integer) * fmv* to access high bits of float register bigger than XLEN * Quiet comparison instructions (fleq/fltq) Zfa defines its instructions in combination with the following extensions: * single-precision floating-point (F) * double-precision floating-point (D) * quad-precision floating-point (Q) * half-precision floating-point (Zfh) Since the RISC-V architecture test framework does not support the RISC-V quad-precision floating-point ISA extension (Q) and the RISC-V half-precision floating-point ISA extension (Zfh), this patch does not include tests for instructions that depend on these extensions. Given missing infrastructure support, the fli.* instruction tests are hand written and not generated. All other test files are generated using riscv-ctg. The generated test files have been generated using the following command (with $BASEISA={rv32i,rv64i}, $FLEN={32,64}), and CFG=$INSN: riscv_ctg.py \ --cgf sample_cgfs/dataset.cgf \ --cgf sample_cgfs/zfa/$CGF \ --base-isa $BASEISA \ --flen $FLEN \ -d tests_$BASEISA_$FLEN/ Exceptions are: * fcvtmod.w.d.cgf is for FLEN=32 only * fmvh.x.d and fmvp.d.x are for BASEISA=rv32i only The resulting tests have been copied to the target directory. The generation of the Zfa test cases depends on a PR in the riscv-ctg repository: riscv-software-src/riscv-ctg#60 The Zfa specification is ratified and can be found here: https://github.com/riscv/riscv-isa-manual/blob/main/src/zfa.adoc Signed-off-by: Christoph Müllner <[email protected]>

cmuellner · 2024-04-02T08:20:57Z

Thanks for spotting the reintroduction of the rtz issue.
I've updated riscv-software-src/riscv-ctg#60 so that this won't happen again.

I've updated this PR with regenerated tests.

allenjbaum · 2024-04-02T15:33:42Z

It's failing the C/I because of changelog issues. I don't know if that's because the version number wasn't updated, or because it was updated, but shouldn't have been though.

…

On Tue, Apr 2, 2024 at 1:21 AM Christoph Müllner ***@***.***> wrote: Thanks for spotting the reintroduction of the rtz issue. I've updated riscv-software-src/riscv-ctg#60 <riscv-software-src/riscv-ctg#60> so that this won't happen again. I've updated this PR with regenerated tests. — Reply to this email directly, view it on GitHub <#330 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJWVFJBL2LROTOYISBTY3JS75AVCNFSM6AAAAAAWVK6RN6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZRGM3DSOJZGY> . You are receiving this because you commented.Message ID: ***@***.***>

Signed-off-by: Allen Baum <[email protected]>

cmuellner · 2024-04-09T19:32:42Z

It's failing the C/I because of changelog issues. I don't know if that's because the version number wasn't updated, or because it was updated, but shouldn't have been though.

This PR shows: "Review required", "All checks have passed", "Merging is blocked".
So, the CI is OK with merging this PR.
But I prefer not to merge this unless riscv-software-src/riscv-ctg#60 is merged (this PR depends on the changes there).

allenjbaum · 2024-04-09T21:48:45Z

Ah, good, I was wondering about that. I won't pull any triggers until that is done.

jamesbeyond · 2024-04-22T15:41:37Z

Add support for the Zfa ISA extension riscv-software-src/riscv-ctg#60 will be merged soon to unblock this one
the missing fmvp.d.x test case will be submitted as a separate PR
The missing RV32E test case for Zfa will be lowered priority, and can be submitted as separate

UmerShahidengr · 2024-05-03T05:46:14Z

I have reviewed and merged riscv-software-src/riscv-ctg#60, so there are no dependecies left for this one. We can merge this one as well, since it has already been reviewed by many people.

Signed-off-by: Umer Shahid <[email protected]>

Added new entry in Changelog and resolved conflicts Signed-off-by: Umer Shahid <[email protected]>

UmerShahidengr · 2024-05-03T05:50:37Z

I have resolved the conflicts by a forced commit.

UmerShahidengr · 2024-05-07T11:46:47Z

As its parallel riscv-ctg PR has been merged, so I am merging this one too.

ptomsich mentioned this pull request Apr 9, 2023

Add Zfa support (excl. quad-precision instructions) riscv/sail-riscv#251

Merged

charmitro mentioned this pull request Apr 10, 2023

Support for Zfa extension riscv-software-src/riscv-config#123

Merged

cmuellner force-pushed the zfa branch from 9f55fa1 to d310dd7 Compare May 10, 2023 22:54

cmuellner force-pushed the zfa branch from d310dd7 to 3ec98ff Compare May 23, 2023 09:57

cmuellner force-pushed the zfa branch from 3ec98ff to e899600 Compare May 23, 2023 10:01

cmuellner force-pushed the zfa branch from e899600 to e7fb08f Compare January 16, 2024 23:04

jordancarlin mentioned this pull request Mar 25, 2024

Correct fcvtmod.w.d flag generation logic for exp == 31 riscv/sail-riscv#442

Merged

cmuellner force-pushed the zfa branch from e7fb08f to 29be788 Compare April 2, 2024 08:18

Merge branch 'main' into zfa

6c62015

Signed-off-by: Allen Baum <[email protected]>

davidharrishmc mentioned this pull request Apr 22, 2024

Zfa tests missing fmvp.d.x #456

Open

UmerShahidengr approved these changes May 3, 2024

View reviewed changes

UmerShahidengr added 2 commits May 3, 2024 10:48

Merge branch 'main' into zfa

6288e0d

Signed-off-by: Umer Shahid <[email protected]>

Update CHANGELOG.md

20e08ad

Added new entry in Changelog and resolved conflicts Signed-off-by: Umer Shahid <[email protected]>

UmerShahidengr merged commit 67c5e71 into riscv-non-isa:main May 7, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the Zfa ISA extension #330

Add support for the Zfa ISA extension #330

cmuellner commented Apr 6, 2023 •

edited

Loading

charmitro commented Apr 7, 2023

pawks commented Apr 10, 2023 •

edited

Loading

cmuellner commented Apr 11, 2023

cmuellner commented May 10, 2023

cmuellner commented May 23, 2023 •

edited

Loading

jscheid-ventana commented Nov 17, 2023

davidharrishmc commented Jan 13, 2024 •

edited

Loading

davidharrishmc commented Jan 13, 2024

davidharrishmc commented Jan 16, 2024 •

edited

Loading

davidharrishmc commented Jan 16, 2024

davidharrishmc commented Jan 16, 2024

cmuellner commented Jan 16, 2024

davidharrishmc commented Jan 17, 2024

davidharrishmc commented Feb 5, 2024

davidharrishmc commented Mar 6, 2024

cmuellner commented Mar 6, 2024

davidharrishmc commented Mar 25, 2024

allenjbaum commented Mar 27, 2024

cmuellner commented Apr 2, 2024

allenjbaum commented Apr 2, 2024 via email

cmuellner commented Apr 9, 2024

allenjbaum commented Apr 9, 2024

jamesbeyond commented Apr 22, 2024

UmerShahidengr commented May 3, 2024

UmerShahidengr commented May 3, 2024

UmerShahidengr commented May 7, 2024

Add support for the Zfa ISA extension #330

Add support for the Zfa ISA extension #330

Conversation

cmuellner commented Apr 6, 2023 • edited Loading

Description

Related Issues

Ratified/Unratified Extensions

List Extensions

Reference Model Used

Mandatory Checklist:

Optional Checklist:

charmitro commented Apr 7, 2023

pawks commented Apr 10, 2023 • edited Loading

cmuellner commented Apr 11, 2023

cmuellner commented May 10, 2023

cmuellner commented May 23, 2023 • edited Loading

jscheid-ventana commented Nov 17, 2023

davidharrishmc commented Jan 13, 2024 • edited Loading

davidharrishmc commented Jan 13, 2024

davidharrishmc commented Jan 16, 2024 • edited Loading

davidharrishmc commented Jan 16, 2024

davidharrishmc commented Jan 16, 2024

cmuellner commented Jan 16, 2024

davidharrishmc commented Jan 17, 2024

davidharrishmc commented Feb 5, 2024

davidharrishmc commented Mar 6, 2024

cmuellner commented Mar 6, 2024

davidharrishmc commented Mar 25, 2024

allenjbaum commented Mar 27, 2024

cmuellner commented Apr 2, 2024

allenjbaum commented Apr 2, 2024 via email

cmuellner commented Apr 9, 2024

allenjbaum commented Apr 9, 2024

jamesbeyond commented Apr 22, 2024

UmerShahidengr commented May 3, 2024

UmerShahidengr commented May 3, 2024

UmerShahidengr commented May 7, 2024

cmuellner commented Apr 6, 2023 •

edited

Loading

pawks commented Apr 10, 2023 •

edited

Loading

cmuellner commented May 23, 2023 •

edited

Loading

davidharrishmc commented Jan 13, 2024 •

edited

Loading

davidharrishmc commented Jan 16, 2024 •

edited

Loading