Re: [PATCH 04/21] target/mips: Explicitly set 2-NaN propagation rule

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 25/10/24 11:12, Peter Maydell wrote:
> Set the 2-NaN propagation rule explicitly in the float_status words
> we use.
> 
> For active_fpu.fp_status, we do this in a new fp_reset() function
> which mirrors the existing msa_reset() function in doing "first call
> restore to set the fp status parts that depend on CPU state, then set
> the fp status parts that are constant".
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/mips/fpu_helper.h       | 22 ++++++++++++++++++++++
>   target/mips/cpu.c              |  2 +-
>   target/mips/msa.c              | 17 +++++++++++++++++
>   fpu/softfloat-specialize.c.inc | 18 ++----------------
>   4 files changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/target/mips/fpu_helper.h b/target/mips/fpu_helper.h
> index ad1116e8c10..7c3c7897b45 100644
> --- a/target/mips/fpu_helper.h
> +++ b/target/mips/fpu_helper.h
> @@ -44,6 +44,28 @@ static inline void restore_fp_status(CPUMIPSState *env)
>       restore_snan_bit_mode(env);
>   }
>   
> +static inline void fp_reset(CPUMIPSState *env)
> +{
> +    restore_fp_status(env);
> +
> +    /*
> +     * According to MIPS specifications, if one of the two operands is
> +     * a sNaN, a new qNaN has to be generated. This is done in
> +     * floatXX_silence_nan(). For qNaN inputs the specifications
> +     * says: "When possible, this QNaN result is one of the operand QNaN
> +     * values." In practice it seems that most implementations choose
> +     * the first operand if both operands are qNaN. In short this gives
> +     * the following rules:
> +     *  1. A if it is signaling
> +     *  2. B if it is signaling
> +     *  3. A (quiet)
> +     *  4. B (quiet)
> +     * A signaling NaN is always silenced before returning it.
> +     */
> +    set_float_2nan_prop_rule(float_2nan_prop_s_ab,
> +                             &env->active_fpu.fp_status);
> +}
> +
>   /* MSA */
>   
>   enum CPUMIPSMSADataFormat {
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index 9724e71a5e0..d0a43b6d5c7 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -407,9 +407,9 @@ static void mips_cpu_reset_hold(Object *obj, ResetType type)
>       }
>   
>       msa_reset(env);
> +    fp_reset(env);
>   
>       compute_hflags(env);
> -    restore_fp_status(env);
>       restore_pamask(env);
>       cs->exception_index = EXCP_NONE;
>   
> diff --git a/target/mips/msa.c b/target/mips/msa.c
> index 61f1a9a5936..9dffc428f5c 100644
> --- a/target/mips/msa.c
> +++ b/target/mips/msa.c
> @@ -49,6 +49,23 @@ void msa_reset(CPUMIPSState *env)
>       set_float_detect_tininess(float_tininess_after_rounding,
>                                 &env->active_tc.msa_fp_status);
>   
> +    /*
> +     * According to MIPS specifications, if one of the two operands is
> +     * a sNaN, a new qNaN has to be generated. This is done in
> +     * floatXX_silence_nan(). For qNaN inputs the specifications
> +     * says: "When possible, this QNaN result is one of the operand QNaN
> +     * values." In practice it seems that most implementations choose
> +     * the first operand if both operands are qNaN. In short this gives
> +     * the following rules:
> +     *  1. A if it is signaling
> +     *  2. B if it is signaling
> +     *  3. A (quiet)
> +     *  4. B (quiet)
> +     * A signaling NaN is always silenced before returning it.
> +     */
> +    set_float_2nan_prop_rule(float_2nan_prop_s_ab,
> +                             &env->active_tc.msa_fp_status);

Alternatively pass float_status* to fp_reset() and call it here as

        mips_fp_reset(&env->active_tc.msa_fp_status);

So we keep the comment in one place.

Regardless,

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>