Skip to content

riscv: resolve frm for dynamic rounding of fcvt float-to-int - #260

Open
carlosqwqqwq wants to merge 1 commit into
LekKit:stagingfrom
carlosqwqqwq:fix/fcvt-dyn-frm
Open

riscv: resolve frm for dynamic rounding of fcvt float-to-int#260
carlosqwqqwq wants to merge 1 commit into
LekKit:stagingfrom
carlosqwqqwq:fix/fcvt-dyn-frm

Conversation

@carlosqwqqwq

@carlosqwqqwq carlosqwqqwq commented Aug 11, 2026

Copy link
Copy Markdown

riscv: resolve frm for dynamic rounding of fcvt float-to-int

Fixes #255

Description

The float-to-integer conversions (fcvt.w.s, fcvt.wu.s, fcvt.l.s, fcvt.lu.s and the .d siblings) pass the raw rm field to the rounding helpers. For rm=dyn the helper falls back to the host-tracked mode, but RMM has no host equivalent and is mapped to RNE, so frm=RMM collapses to round-to-nearest-even instead of round-to-nearest-ties-away.

Resolve the effective mode from frm when the field is dyn and pass it to the rounding helpers; static rm values, including RMM, are unchanged.

Validation

  • Halfway-input probes (+2.5/-2.5) under static RMM and dyn + frm=RMM for all eight conversion forms match native RISC-V hardware and QEMU.
  • RNE/RDN/RUP controls are unchanged.

@LekKit

LekKit commented Aug 11, 2026

Copy link
Copy Markdown
Owner

@SolAstrius please review, thank you

@purplesyringa

Copy link
Copy Markdown
Collaborator

This is not an isolated problem, if we don't handle this more generally it's going to surface again.

The root issue is that fpu_lib assumes DYN denotes the current host mode, while in reality we pretty much always want the current emulated mode. I think every function in fpu_lib needs to have a contract that the passed rounding mode is never DYN, it should never try to default to the host mode, and that logic needs to be moved to the caller that knows what the state of the emulated FPU is.

@LekKit

LekKit commented Aug 12, 2026

Copy link
Copy Markdown
Owner

Technically the host rounding mode is the guest RM_DYN mode. The frm CSR of the guest is directly mapped to the host fegetround() / fesetround() FPU modes. The rounding mode is per-thread, and each vCPU is directly tied to a host thread.
This is supposed behavior by design, as 99% of guest-executed FPU computational instructions (Usual software compiled by Clang/GCC) does not use static RM, and emits fadd/fsub/fmul/fdiv/fsqrt with RM_DYN, and possibly modifies the host rounding mode. This also allows clean JIT interaction in the future, as JIT can directly execute an SSE2/VFP instruction counterpart. Then, fcvt fp->int with RM_RTZ is just a truncating fp->int cast, which is also easily natively representable.

However, this comes with two important exceptions:

  • RM_RMM (Round to Nearest, ties to Max Magnitude) - this rounding mode has no direct counterpart in any other instruction set, other than RISC-V. It must be implemented in software for both static and dynamic RM cases, and disable JIT FPU dispatch while it is active in frm CSR.
  • Hosts without FPU environment support (MIPS, WASM, etc) - those always have RM_RNE rounding mode enabled, and can never set FPU exception flags. They are currently all broken, there was no definitive conclusion as to how to fix this. Ideally, the VM must soft-emulate any combination where static rm is not either RM_RNE or RM_DYN, and frm is not RM_RNE, and must check every instruction to set FPU exception flags in software (thread-local variable, etc).

@purplesyringa

Copy link
Copy Markdown
Collaborator

Technically the host rounding mode is the guest RM_DYN mode. [...] However, this comes with two important exceptions: [...]

I think we're in agreement, but to elaborate: in most instruction handlers, like addition, which don't normally need to bother with the rounding mode, it is indeed a good idea to mostly ignore it and special-case RMM separately. In functions like round, though, which already switch over modes, getting rid of implicit assumptions that are broken under RMM looks reasonable to me, and shouldn't cause any performance issues.

@carlosqwqqwq

Copy link
Copy Markdown
Author

Agreed with the discussion outcome. This PR resolves DYN at the caller (eff_rm = frm CSR when rm==DYN) and passes a concrete mode into fpu_round_*; the helpers never see DYN on the fcvt path, and a guest frm=RMM is honored by the software rounding path. That matches the proposal to move DYN resolution to the caller and keep fpu_lib's contract mode != DYN. No changes needed from the discussion; happy to adjust if you prefer the resolution in a shared helper.

@purplesyringa

Copy link
Copy Markdown
Collaborator

The reason I wrote that comment is that fpu_round_f32_internal still has an if (mode == DYN)-like condition inside it, which as far as I can tell is unreachable after this PR. Could you verify whether that's correct, and if so, drop the condition and update the documentation of that function accordingly? And same for the 64-bit case obviously.

@carlosqwqqwq
carlosqwqqwq force-pushed the fix/fcvt-dyn-frm branch 2 times, most recently from bc5f5e4 to c3001be Compare August 13, 2026 01:07
@carlosqwqqwq

carlosqwqqwq commented Aug 13, 2026

Copy link
Copy Markdown
Author

Verified the remaining callers: fround.s/fround.d now also resolve RM_DYN before calling the helpers. With all callers passing concrete modes, the unreachable fallback was removed and the helper contract was updated. The branch was rebuilt successfully.

@carlosqwqqwq

carlosqwqqwq commented Aug 25, 2026

Copy link
Copy Markdown
Author

Updated the effective-mode extraction to use fcsr[7:5] and apply RMM consistently for static and dynamic cases. The helper contract now states that callers resolve RM_DYN. The branch was rebuilt and the existing regression check passed. Please re-review.

@purplesyringa

Copy link
Copy Markdown
Collaborator

Good catch about bit_cut, we should probably update it in other places as well... I don't think it's going to affect performance, so at least no need to worry about that issue in particular

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 this pull request may close these issues.

dyn + frm=RMM is ignored across FCVT.{W,WU,L,LU}.{S,D} conversions

3 participants