Correct fcvtmod.w.d flag generation logic for exp == 31 #442
Conversation
|
Are you aware of any ACT tests that test this particular case?
I suspect not, because no one has complained that Spike vs. Sail are
diverged.
If that is the case, could you file an ACT issue?
…On Mon, Mar 25, 2024 at 12:59 PM Jordan Carlin ***@***.***> wrote:
As described in issues #388
<#388> and #268
<#268>, fcvtmod.w.d is
producing an inexact instead of invalid flag for numbers with an exponent
of 31 that are not representable as a two's complement integer (> 2^31 - 1
or < -2^31).
Per #388 <#388>,
According to the Zfa specification, "Floating-point exception flags are
raised the same as they would be for FCVT.W.D with the same input operand."
According to the unprivileged specification Table 11.4, the invalid
exception should be set for inputs > 2^31 - 1 or < -2^31.
This is resolved by adding an additional check when determining the
invalid flag for if the integer is > 2^31 - 1 or < -2^31. A similar
approach is currently used by Spike.
------------------------------
You can view, comment on, or merge this pull request online at:
#442
Commit Summary
- e2e29f2
<e2e29f2>
Correct fcvtmod.w.d flag generation logic
File Changes
(1 file <https://github.com/riscv/sail-riscv/pull/442/files>)
- *M* model/riscv_insts_zfa.sail
<https://github.com/riscv/sail-riscv/pull/442/files#diff-aee727463f4fb34d39434b987d074ce2b8501046e052954d07d30a38c9e1dca6>
(4)
Patch Links:
- https://github.com/riscv/sail-riscv/pull/442.patch
- https://github.com/riscv/sail-riscv/pull/442.diff
—
Reply to this email directly, view it on GitHub
<#442>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJRDQPMS2HF6JVFMCBLY2B62PAVCNFSM6AAAAABFHURGLSVHI2DSMVQWIX3LMV43ASLTON2WKOZSGIYDMNJYG42DONI>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
model/riscv_insts_zfa.sail
Outdated
| let flags : bits(5) = if (true_exp > 31) then nvFlag() | ||
| let max_integer = if sign == 0b1 then unsigned(0x80000000) | ||
| else unsigned(0x7fffffff); | ||
| let flags : bits(5) = if (true_exp > 31 | unsigned(integer) > max_integer) then nvFlag() |
There was a problem hiding this comment.
Merging this into a single condition for the if makes the specification harder to read:
- the check for an exponent >31 means that we shifted bits out of the integer-part of the fixed-point representation;
- while the comparison against max_integer checks whether the number is representable.
So I would rahter to write this as
if true_exp > 31 then nvFlag()
else if not(is_representable_as_signed32(integer)) then nvFlag()
...
Thinking somewhat more about this: could we, when converting this into twos-complement, handle this by producing a 33bit intermediate result and checking whether that intermediate result fits into 32bit?
This would remove the need to set the max_integer limit...
There was a problem hiding this comment.
The switch to two different if statements for improved readability sounds good to me. I'll go ahead and make that change. I'm not very familiar with Sail, so I'm not sure about implementing the 33bit intermediate to eliminate max_integer, but I can definitely take a look.
There was a problem hiding this comment.
Per your suggestion, I split the check into two conditions to improve readability. I can continue to look into eliminating max_integer tomorrow if we want to go that route.
There was a problem hiding this comment.
I was unable to find a way to do it without max_integer, but hopefully the intent is pretty clear in the current implementation.
I believe this question was raised in the Spike case already — did it get resolved back then? |
There is an open PR to add Zfa (including fcvtmod.w.d) tests. riscv-non-isa/riscv-arch-test/pull/330 |
|
When I look that test up, it has rm=dyn instead of rm=rtz, so I don't know
if we are comparing the same thing, or if it makes a difference.
I would like to see the smallest number that overflows and the largest that
doesn't, and ensure that the correct thing happens with all rounding modes.
…On Mon, Mar 25, 2024 at 2:02 PM Jordan Carlin ***@***.***> wrote:
Are you aware of any ACT tests that test this particular case? I suspect
not, because no one has complained that Spike vs. Sail are diverged. If
that is the case, could you file an ACT issue?
I believe this question was raised in the Spike case already — did it get
resolved back then?
There is an open PR to add Zfa (including fcvtmod.w.d) tests.
riscv-non-isa/riscv-arch-test#330
<riscv-non-isa/riscv-arch-test#330>
The fcvtmod.w.d_b22-01.S test has a case that causes this discrepancy.
inst_34:// fs1 == 1 and fe1 == 0x41e and fm1 == 0xe9b7e5fc9eba4 and fcsr == 0x0 and rm_val == 7
/* opcode: fcvtmod.w.d ; op1:f31; dest:x31; op1val:0xc1ee9b7e5fc9eba4; valaddr_reg:x8;
val_offset:10*FLEN/8; rmval:rtz; correctval:??; testreg:x6;
fcsr_val:0*/
TEST_FPID_OP(fcvtmod.w.d, x31, f31, rtz, 0, 0, x8, 10*FLEN/8, x9, x5, x6,FLREG)
—
Reply to this email directly, view it on GitHub
<#442 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJWUAY4CPJDDT7NWZ5LY2CGGHAVCNFSM6AAAAABFHURGLSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJYHEYDQOBTG4>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
|
do you have a pointer to the version that changed dyn to rtz, because the
dyn version is the one I found.
But, if that's the case, we don't need to check any other rounding modes,
and the Sail fix is the only thing that needs changing.
…On Mon, Mar 25, 2024 at 2:48 PM Philipp Tomsich ***@***.***> wrote:
When I look that test up, it has rm=dyn instead of rm=rtz, so I don't know
if we are comparing the same thing, or if it makes a difference. I would
like to see the smallest number that overflows and the largest that
doesn't, and ensure that the correct thing happens with all rounding modes.
fcvtmod.w.d is not defined for rounding modes other than rtz (not that
the encoding is selective on 0b001 where the rounding mode field is):
mapping clause encdec = RISCV_FCVTMOD_W_D(rs1, rd) if haveDExt() & haveZfa()
<-> 0b110_0001 @ 0b01000 @ rs1 @ 0b001 @ rd @ 0b101_0011 if haveDExt() & haveZfa()
—
Reply to this email directly, view it on GitHub
<#442 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJSOSLGAAP55KKXV3J3Y2CLT5AVCNFSM6AAAAABFHURGLSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJYHE3TONBXGA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
See https://github.com/riscv/riscv-isa-manual/blob/main/src/zfa.adoc |
From the spec:
|
|
Ooops. I wasn't clear. I was referring to the test itself, not the spec.
The version I was seeing for that particular test (and others) all used
dynamic rounding.
the newest version I see is commit e899600
<riscv-non-isa/riscv-arch-test@e899600>
to e7fb08f
<riscv-non-isa/riscv-arch-test@e7fb08f>
Compare
<https://github.com/riscv-non-isa/riscv-arch-test/compare/e899600de5e1845d60a5496fcbba883301765a72..e7fb08f1c1bd9a65d5f3d27cb7fce7ef0dd21eb8>2
months ago
<riscv-non-isa/riscv-arch-test#330 (comment)>
here:
riscv-non-isa/riscv-arch-test@e7fb08f#diff-6eccefabdbfd6365397a5c8da35acd14f461855ae926d6e52b1e8fde8a8d60d9
and looking at the file rv32i_m/D_Zfa/src/fcvtmod.w.d_b22-01.S
Of course, being the git incompetent, I could be looking in the wrong
place.
…On Mon, Mar 25, 2024 at 3:17 PM Philipp Tomsich ***@***.***> wrote:
do you have a pointer to the version that changed dyn to rtz, because the
dyn version is the one I found. But, if that's the case, we don't need to
check any other rounding modes, and the Sail fix is the only thing that
needs changing.
See https://github.com/riscv/riscv-isa-manual/blob/main/src/zfa.adoc
From the spec:
This instruction is only provided if the D extension is implemented. 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.
—
Reply to this email directly, view it on GitHub
<#442 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJT67RJYN3GCXCILP2DY2CPA5AVCNFSM6AAAAABFHURGLSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJZGAYTMNJTGA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
There is an issue with the tests generated in the current version of riscv-non-isa/riscv-arch-test#330. Based on the discussion there, it seems this is a known issue with riscv-ctg right now. There was a version that had rtz at some point, but I can't seem to find it now. |
|
I fixed the CFG in riscv-ctg to reliably generate |
|
ACT tests have been updated and the proposed code change was modified to improve readability. |
|
I'd love to get this into Sail so that riscof stops throwing errors on fcvtmod.w.d. Is there anything that still needs to be resolved to accept this PR? |
|
We need to have at least 2 people (not the author) review and approve this PR. |
|
I'm not a floating point expert but this looks good to me, and it appears to fix a real problem with the current behaviour others are having. |
jordancarlin
force-pushed
the
master
branch
2 times, most recently
from
a74578d
to
c86bc83
Compare
|
I'm not a Sail expert, but the math agrees with Spike. |
|
Seems like we're starting to get some reviews (both formal and not) on this. Would love to get this merged so that these issues can be resolved. @billmcspadden-riscv |
|
@billmcspadden-riscv, We didn't get to it in the meeting but I think this and a bunch of other small fix PRs are waiting to be merged. |
Fixes #388 and fixes #268. fcvtmod.w.d is producing an inexact instead of invalid flag for numbers with an exponent of 31 that are not representable as a two's complement integer (> 2^31 - 1 or < -2^31).
Per #388,
This is resolved by adding an additional check when determining the invalid flag for if the integer is > 2^31 - 1 or < -2^31. A similar approach is currently used by Spike.