riscv_fpu: static/FMA rounding modes, exact integral rounding, FMA underflow tininess#246
Draft
SolAstrius wants to merge 4 commits into
Draft
riscv_fpu: static/FMA rounding modes, exact integral rounding, FMA underflow tininess#246SolAstrius wants to merge 4 commits into
SolAstrius wants to merge 4 commits into
Conversation
fpu_round_fXX_internal biased the value by +/-0.5 (or +/-1.0) with a real FP add and let the caller's int cast truncate. The add is itself a rounded op, which broke it two ways: it raised a spurious INEXACT for the discarded fraction (leaking NX alongside e.g. the NV of an out-of-range fcvt), and it mis-rounded whenever the biasing add was inexact or exact on the wrong side -- rne(prev(0.5)) returned 1 because prev(0.5) + 0.5 rounds up to 1.0, and rne(2.5) returned 3 because 2.5 + 0.5 is exactly 3.0, i.e. the trick implements ties-away, not ties-to-even. Decide on the fraction bits of the encoding instead: no host FP arithmetic, so no flag leaks, no double rounding, and no dependence on the host rounding mode (which USE_SOFT_FENV hosts don't have). RMM is now a first-class case -- only DYN falls back to the tracked mode, so an explicit rmm request is honored instead of being swapped for the dynamic mode. Verified bit-exact against host rint()/round() in all four modes over the full fractional exponent range of both widths (2.0e9 cases). Signed-off-by: Sol Astrius Phoenix <sol@astrius.ink>
Arithmetic and conversion ops only ever ran in the mode the frm CSR left on the host FPU; a static rm field (fadd.s ...,rtz etc.) was silently ignored. Apply a differing static mode around the op, gated on the op actually being rounding-capable: funct3 is an rm field only there, while on fsgnj/fmin/fmax/fcmp/fclass/fmv it encodes the operation itself and must not drive the host mode. Ops that take rm as an explicit argument (fcvt to integer, fround) consume the field directly and need no wrap. A static rmm field runs in RNE for now, which differs from roundTiesToAway only on exact halfway ties; those are covered once the exact RMM fixups land (LekKit#242). The dynamic-RMM preparation is gated to rm == DYN so it no longer fires under a static field. Signed-off-by: Sol Astrius Phoenix <sol@astrius.ink>
fpu_fma rounds in the host mode, so a static rm field on fmadd/fmsub/fnmsub/fnmadd was silently computed in whatever mode frm left set. Route the FMA ops through riscv_fma32/64, which apply a differing static mode around the op using the same riscv_fpu_host_rm rule as the OP-FP dispatch. As there, a static rmm field runs in RNE until the exact ties-away fixups (LekKit#242) extend to FMA. Signed-off-by: Sol Astrius Phoenix <sol@astrius.ink>
IEEE (and RISC-V) underflow means tiny after rounding: the result, rounded with an unbounded exponent range, lies below the minimum normal. Hosts that detect tininess before rounding (e.g. aarch64) disagree exactly when an FMA result lands on +/- the minimum normal, flagging a spurious UF for a result that rounded up out of the tiny range. On that boundary, recompute the after-rounding verdict and force the flag: - f32: the exact product widens into f64, the sum rounds once at 53 bits, and the scaled conversion once at 24; 53 >= 2*24 + 2 makes the double rounding innocuous, so the scaled magnitude compared against 1.0 is the unbounded-exponent verdict. - f64: no wider type exists, so rescale the op by 2^52 into the normal range, where the single fused rounding is already unbounded- equivalent (safety bounds are derived in a comment; an overflowing a*2^52 for b == 0 yields NaN, which correctly reads as not-tiny). The verdict merges over a flag snapshot from before the op, so a sticky UF from earlier ops is preserved and nothing the recompute raises can leak. fpu_fma64 is split into a bare fpu_fma64_raw plus the flag handling so the f64 fixup can reuse the fused op without re-entering it. On aarch64, fma(2^-62*(1-2^-23), 2^-64*(1+2^-23), 0) previously returned the correct 2^-126 but flagged NX|UF instead of NX; the f64 analog and the negative side misbehaved the same way. Signed-off-by: Sol Astrius Phoenix <sol@astrius.ink>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reworked resubmission of the remaining #239 commits, addressing @purplesyringa's review. Four commits, each standalone on
staging; no dependency on #242 (RMM), and the two deliberately overlap nowhere except the sharedriscv_fpu_host_rmhelper.1.
fpu_lib: round to integral values on the encoding, not via biasing addsReplaces the NACK'd INEXACT patch. The review showed the old
f + 0.5trick was broken beyond the flag leak (this comment): the biasing add itself rounds, sorne(prev(0.5))returned 1 andrne(2.5)returned 3 (ties-away, not ties-to-even).fpu_round_fXX_internalnow decides on the fraction bits of the encoding: no host FP arithmetic, so no spurious flags, no double rounding, and no dependence on a host rounding mode (relevant for the futureUSE_SOFT_FENVstory). RMM is a first-class case — only DYN falls back to the tracked mode, sofcvt/froundwith a staticrmmfield round correctly.Verified bit-exact against host
rint()/round()in all four modes over the full fractional exponent range of both widths (2.0e9 cases, zero mismatches).2.
riscv_fpu: honor the static rounding-mode fieldReworked per review: the mode change is now gated per-instruction (
riscv_f_op_is_rounding) — funct3 is an rm field only on rounding-capable ops, sofsgnj/fcmp/fclass/fmvnever trigger it. Ops that consumermas an argument (fcvt-to-int, fround) need no wrap. A staticrmmfield runs in RNE for now (differs only on exact ties; wired to the exact fixups after #242 lands). The dynamic-RMM preparation is gated torm == DYNso a static field is no longer clobbered by it.3.
riscv_fpu: honor the rounding mode in the FMA opsThe fnmadd operand-negation part already landed via #241; this is the rm plumbing, now sharing
riscv_fpu_host_rmwith the OP-FP dispatch instead of duplicating the effective-mode logic (review ask).4.
fpu_lib: set the FMA underflow flag by IEEE after-rounding tininessReworked per review, and moved into
fpu_fma32/64proper (fixingfpu_fma32together with the patch, as asked):b == 0overflow-to-NaN case reads correctly as not-tiny;On aarch64 (before-rounding tininess),
fma(2^-62*(1-2^-23), 2^-64*(1+2^-23), 0)returned the correct 2^-126 but flagged NX|UF instead of NX; targeted tests cover both boundary directions, an exact boundary, a genuinely-tiny-at-boundary case, and sticky-UF preservation — all pass with the fix, three fail without it.Conformance (riscv-arch-test ACT4, Spike reference)
No regressions. The remaining F/D failures are, by sampled diagnosis, exactly the two intentionally-excluded categories:
riscv_prepare_rmmdirected hack (e.g. it reads a NaN-boxed f32 through the f64default:path and rounds fsqrt the wrong way). Deleted and replaced by riscv_fpu: RMM (roundTiesToAway) for the scalar OP-FP ops #242.fadd.s→0x7fd825fevs0x7fc00000). That's the separate flag-gated patch we agreed on; needs a decision on compile-time vs runtime flag first.A local combined build (this PR + #242 + the canonicalization patch) reaches 244/260, and every one of its 16 residual failures is an FMA test failing on an
frm=rmmtie sub-case — that's the one #239 commit not yet resubmitted anywhere ("round the FMA family to nearest, ties-away under RMM"); it needs #242's fixup machinery, so it goes on top of #242 once that merges, closing the gap to the 260/260 the old chain reported.cc @purplesyringa — draft until reviewed, to keep GitHub from merging things on its own again.