Re: [PATCH 2/2] target/ppc: Fix FPSCR.FI changing in float_overflow_excp()

[Richard Henderson]

On 5/9/22 07:48, Víctor Colombo wrote:
> This patch fixes another not-so-clear situation in Power ISA
> regarding the inexact bits in FPSCR. The ISA states that:
>
> """
> When Overflow Exception is disabled (OE=0) and an
> Overflow Exception occurs, the following actions are
> taken:
> ...
> 2. Inexact Exception is set
> XX <- 1
> ...
> FI is set to 1
> ...
> """
>
> However, when tested on a Power 9 hardware, some instructions that
> trigger an OX don't set the FI bit:
>
> xvcvdpsp(0x4050533fcdb7b95ff8d561c40bf90996) = FI: CLEARED -> CLEARED
> xvnmsubmsp(0xf3c0c1fc8f3230, 0xbeaab9c5) = FI: CLEARED -> CLEARED
> (just a few examples. Other instructions are also affected)
>
> The root cause for this seems to be that only instructions that list
> the bit FI in the "Special Registers Altered" should modify it.
>
> QEMU is, today, not working like the hardware:
>
> xvcvdpsp(0x4050533fcdb7b95ff8d561c40bf90996) = FI: CLEARED -> SET
> xvnmsubmsp(0xf3c0c1fc8f3230, 0xbeaab9c5) = FI: CLEARED -> SET
>
> (all tests assume FI is cleared beforehand)
>
> Fix this by passing an argument to float_overflow_excp() indicating
> if the FI should be set.
>
> Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br>
> ---
>   target/ppc/fpu_helper.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> index 773c80e12d..ee1259ede1 100644
> --- a/target/ppc/fpu_helper.c
> +++ b/target/ppc/fpu_helper.c
> @@ -329,7 +329,7 @@ static inline void float_zero_divide_excp(CPUPPCState *env, 
> uintptr_t raddr)
>       }
>   }
>   
> -static inline void float_overflow_excp(CPUPPCState *env)
> +static inline void float_overflow_excp(CPUPPCState *env, bool set_fi)
>   {
>       CPUState *cs = env_cpu(env);
>   
> @@ -345,7 +345,9 @@ static inline void float_overflow_excp(CPUPPCState *env)
>           env->error_code = POWERPC_EXCP_FP | POWERPC_EXCP_FP_OX;
>       } else {
>           env->fpscr |= FP_XX;
> -        env->fpscr |= FP_FI;
> +        if (set_fi) {
> +            env->fpscr |= FP_FI;
> +        }


Again, I believe setting FP_FI here is wrong, it should only be set later in 
do_float_check_status.  Indeed, setting XX here probably isn't best...
..
> @@ -471,7 +473,7 @@ static void do_float_check_status(CPUPPCState *env, bool 
> change_fi,
>       int status = get_float_exception_flags(&env->fp_status);
>   
>       if (status & float_flag_overflow) {
> -        float_overflow_excp(env);
> +        float_overflow_excp(env, change_fi);

I think the ideal solution would be to return an update to status from float_overflow_excp 
so that all of the inexact handling happens below.  Since inexact is the last bit to be 
processed, this could be as simple as

    if (status & overflow) {
        status = float_overflow_excp(env);
    } else if (status & underflow) {
        ...
    }
    if (status & inexact) {
    ...

returning OE ? 0 : float_flag_inexact, without trying to merge inexact into the full set 
of status flags.


r~