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

mixed and non-standard single preciscion implementation #32

Open
lenawanel opened this issue May 27, 2024 · 0 comments · May be fixed by #36
Open

mixed and non-standard single preciscion implementation #32

lenawanel opened this issue May 27, 2024 · 0 comments · May be fixed by #36

Comments

@lenawanel
Copy link

The riscv spec clearly states that

"When multiple floating-point precisions are supported, then valid values of narrower n-bit types,
n<FLEN, are represented in the lower n bits of an FLEN-bit NaN value, in a process termed NaN-
boxing. The upper bits of a valid NaN-boxed value must be all 1s. Valid NaN-boxed n-bit values
therefore appear as negative quiet NaNs (qNaNs) when viewed as any wider m-bit value, n < m ≤
FLEN. Any operation that writes a narrower result to an 'f' register must write all 1s to the uppermost
FLEN-n bits to yield a legal NaN-boxedvalue"

[1]

It specifically states that

"Floating-point n-bit transfer operations move external values held in IEEE standard formats into and
out of the f registers, and comprise floating-point loads and stores (FLn/FSn) and floating-point move
instructions (FMV.n.X/FMV.X.n). A narrower n-bit transfer, n<FLEN, into the f registers will create a
valid NaN-boxed value. A narrower n-bit transfer out of the floating-point registers will transfer the
lower n bits of the register ignoring the upper FLEN-n bits"

[1]

, yet this project deals with narrower stores (fsw) by performing a double to single precision conversion,

rvemu/src/cpu.rs

Line 1664 in f55eb5b

self.write(addr, (self.fregs.read(rs2) as f32).to_bits() as u64, WORD)?

with narrower loads (flw) by performing a single to double precision conversion,

rvemu/src/cpu.rs

Lines 1407 to 1408 in f55eb5b

let val = f32::from_bits(self.read(addr, WORD)? as u32);
self.fregs.write(rd, val as f64);

confusingly implements the fmv.x.w according to standard,

rvemu/src/cpu.rs

Lines 3116 to 3120 in f55eb5b

self.xregs.write(
rd,
(self.fregs.read(rs1).to_bits() & 0xffffffff) as i32 as i64
as u64,
);

and doesn't NaN-box the value when emulating the fmv.w.x instruction.

rvemu/src/cpu.rs

Lines 3201 to 3202 in f55eb5b

self.fregs
.write(rd, f64::from_bits(self.xregs.read(rs1) & 0xffffffff));

The spec further states about other floating point operations that

"Apart from transfer operations described in the previous paragraph, all other floating-point operations
on narrower n-bit operations, n<FLEN, check if the input operands are correctly NaN-boxed, i.e., all
upper FLEN-n bits are 1. If so, the n least-significant bits of the input are used as the input value,
otherwise the input value is treated as an n-bit canonical NaN."

[1]

however this project implements single precision floating point operations like fadd.s by performing double to single precision conversion.

rvemu/src/cpu.rs

Lines 2642 to 2645 in f55eb5b

self.fregs.write(
rd,
(self.fregs.read(rs1) as f32 + self.fregs.read(rs2) as f32) as f64,
)

If the non-standard behavior is intentional, at least the fmv.w.x instruction should be updated to perform a single to double precision conversion, as the current logic can and will have confusing results when moving floats that for example could be constructed in integer registers first.

[1] (https://github.com/riscv/riscv-isa-manual/releases/download/20240411/unpriv-isa-asciidoc.pdf, page 120, section 21.2. "NaN Boxing of Narrower Values")

lenawanel added a commit to lenawanel/rvemu that referenced this issue May 29, 2024
@lenawanel lenawanel linked a pull request May 29, 2024 that will close this 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.

1 participant