Re: [PATCH] target/sparc: emulate floating point queue when raising fp traps

[Richard Henderson <richard.henderson@linaro.org>]

On 8/14/24 09:22, Carl Hauser wrote:
> I am unable to test the linux and NetBSD behavior because I've been
> unable to get gcc to actually generate quad precision instructions on those
> systems.

-mhard-quad-float

> +
> +    /* single-element FPU fault queue */
> +    uint32_t fsr_qne;                /* qne */
> +    uint32_t fsr_qi;                 /* faulting fp instruction */
> +    target_ulong fsr_qa;             /* address of faulting instruction */

I suppose these could be sparc32 only, but not critical,
and it would require ifdefs elsewhere as well.

> diff --git a/target/sparc/insns.decode b/target/sparc/insns.decode
> index fbcb4f7aef..e0ad090340 100644
> --- a/target/sparc/insns.decode
> +++ b/target/sparc/insns.decode
> @@ -644,8 +644,9 @@ STF         11 ..... 100100 ..... . .............       @r_r_ri_na
>   STFSR       11 00000 100101 ..... . .............          @n_r_ri
>   STXFSR      11 00001 100101 ..... . .............          @n_r_ri
>   {
> -  STQF      11 ..... 100110 ..... . .............          @q_r_ri_na
> -  STDFQ     11 ----- 100110 ----- - -------------
> +
> +  STDFQ     11 ..... 100110 ..... . .............          @r_r_r_asi ## SPARC-V7-8
> +  STQF      11 ..... 100110 ..... . .............          @q_r_ri_na ## SPARC-V9
>   }

There is an existing bug:

- TRANS(STQF, ALL, do_st_fpr, a, MO_128)
+ TRANS(STQF, 64, do_st_fpr, a, MO_128)

which prevented the current ordering from working.  With the fix, just add the argument 
set for STDFQ (and the comments, I suppose).


> @@ -147,6 +148,20 @@ void sparc_cpu_do_interrupt(CPUState *cs)
>       env->psret = 0;
>       cwp = cpu_cwp_dec(env, env->cwp - 1);
>       cpu_set_cwp(env, cwp);
> +#ifndef TARGET_SPARC64
> +    if (intno == TT_FP_EXCP) {
> +        env->fsr_qne = FSR_QNE;
> +        env->fsr_qa = env->pc;
> +        env->fsr_qi = cpu_ldl_code(env, env->fsr_qa);
> +         /*
> +          * Because of the asynchronous FPU on real Sparc 32 bit
> +          * machines, the pc and npc will have already been advanced
> +          * by the time that the trap is taken.
> +          */
> +        env->pc = env->npc;
> +        env->npc = env->npc + 4;
> +    }
> +#endif

No need for the ifdef -- the whole file is sparc32 only.

> @@ -1458,8 +1459,10 @@ static void gen_op_fpexception_im(DisasContext *dc, int ftt)
>        * or when raising FSR_FTT_IEEE_EXCP, i.e. check_ieee_exception.
>        * Thus we can simply store FTT into this field.
>        */
> +
>       tcg_gen_st_i32(tcg_constant_i32(ftt), tcg_env,
>                      offsetof(CPUSPARCState, fsr_cexc_ftt));
> +
>       gen_exception(dc, TT_FP_EXCP);
>   }
> 

Drop the unrelated whitespace.

> -static bool trans_STDFQ(DisasContext *dc, arg_STDFQ *a)
> +static bool trans_STDFQ(DisasContext *dc, arg_r_r_ri_asi *a)
>   {
> +
> +#ifndef TARGET_SPARC64
>       if (!avail_32(dc)) {
>           return false;
>       }
> @@ -4538,10 +4543,19 @@ static bool trans_STDFQ(DisasContext *dc, arg_STDFQ *a)
>       if (gen_trap_ifnofpu(dc)) {
>           return true;
>       }
> -    gen_op_fpexception_im(dc, FSR_FTT_SEQ_ERROR);
> -    return true;
> +
> +    TCGv store_addr = gen_ldst_addr(dc, a->rs1, a->imm, a->rs2_or_imm);
> +    if (store_addr == NULL) {
> +        return false;
> +    }
> +    gen_helper_store_fp_queue(tcg_env, store_addr);
> +    return advance_pc(dc);
> +#else
> +    g_assert_not_reached();
> +#endif
>   }

You shouldn't need the ifdef here, because (1) avail_32 should make the whole function 
vanish for sparc64 and (2) you handled gen_helper_store_fp_queue with the 
qemu_build_not_reached define at the top of the file.

Otherwise, this looks really good.


r~