On 2026-01-10 13:57, Richard Henderson wrote: > (0) We really should clean up this code so that there's not so much > duplication. > FOO_is_quiet_nan and FOO_is_signaling_nan really should share code. > That would have caught the above. >
Thanks for the suggestion and I think that maybe we can remove the FOO_is_[quiet|signaling]_nan functions here. These OCP FP8 nan checkings should be different implemntation defined behaviors. > (1) RISC-V always uses default nan mode, the OCP spec declines to define > SNaN vs QNaN, leaving the 8 unique NaN encodings unspecified, and RISC-V > does not do so either. You assert later: > > + * RISC-V uses only quiet NaNs in its OCP FP8 implementation. > > Is this out-of-band discussion with engineers? > Because it's missing from the (remarkably short) document. > The RISC-V Zvfofp8min extension specification (v0.2.1) explicitly states the NaN handling behavior for OFP8 conversions: 1. Canonical NaN Definition (Section: Zvfofp8min): "The canonical NaN for both E4M3 and E5M2 is 0x7f." 2. Widening Conversion Behavior (vfwcvtbf16.f.f.v instruction): "No rounding occurs, and no floating-point exception flags are set." The specification's explicit statement that "no floating-point exception flags are set" for vfwcvtbf16.f.f.v provides clear justification for treating all OFP8 NaNs as quiet in this specific context. 3. Narrowing Conversion Behavior (vfncvtbf16.f.f.w instruction): "Since E4M3 cannot represent infinity, infinite results are converted to the canonical NaN, 0x7f." This demonstrates that RISC-V uses quiet NaN propagation semantics throughout the OFP8 conversion pipeline. > (2) Arm does specify (see FP8Unpack in the ARM pseudocode), doing the usual > thing in taking the msb of the mantissa for SNaN. Which means that E4M3 is > *always* SNaN. > > Both architectures then immediately convert to FP16 default nan, however Arm > *does* raise invalid operand exception for the SNaN, so we can't just ignore > it. > > Given that there's exactly one RISC-V instruction for which this matters, > vfwcvtbf16.f.f.v, it seems like it might be better to simply adjust > float_status.no_signaling_nans within the helper rather than introduce > ocp_fp8e5m2_no_signal_nan. > > > + /* > > + * When true, OCP FP8 formats use the same canonical NaN representation > > + * (0x7F) for all NaN outputs. RISC-V specifies a single canonical NaN > > + * for both E4M3 and E5M2. > > + */ > > + bool ocp_fp8_same_canonical_nan; > > Similarly you could adjust the canonical nan around the 4 FP16->FP8 > conversion insn helpers: > > /* Default NaN value: sign bit clear, all frac bits set */ > set_float_default_nan_pattern(0b01111111, &env->fp_status); > > In either case, "bool" doesn't seem appropriate. > > FWIW, Arm retains the msb set pattern as for all other fp formats > (FP8DefaultNaN). > > > r~ Thank you for the review feedbacks and suggestions. The suggestion to handle the canonical nan and quiet nan within the helper function rather than adding global state to float_status is the cleaner solution. I will incorporate this change in v2 of the patchset. Thanks a lot, rnax
