Re: [PATCH RESEND 04/10] target/ppc: Move mffsce to decodetree

[Richard Henderson]

On 5/17/22 09:47, Víctor Colombo wrote:
> -static void do_mffsc(int rt, uint64_t set_mask)
> +static void do_mffsc(int rt, TCGv_i64 t1, uint64_t set_mask,
> +                     uint64_t clear_mask, uint32_t fpscr_mask)
>   {
>       TCGv_i64 fpscr;
>   
> @@ -618,6 +619,12 @@ static void do_mffsc(int rt, uint64_t set_mask)
>       tcg_gen_andi_i64(fpscr, fpscr, set_mask);
>       set_fpr(rt, fpscr);
>   
> +    if (fpscr_mask) {
> +        tcg_gen_andi_i64(fpscr, fpscr, clear_mask);

The naming doesn't seem right for clear_mask.  I would expect clear_mask to contain the 
bits that we want to remove, and the computation to be

   fpscr &= ~clear_mask

> +        tcg_gen_or_i64(fpscr, fpscr, t1);

Can we get a better name than t1?  Also, I think perhaps NULL would be better for when we 
do not wish to include this.  I suppose tcg_constant_i64(0) is *probably* already in the 
constant hash table, but why look it up at all?

> +        gen_helper_store_fpscr(cpu_env, fpscr, tcg_constant_i32(fpscr_mask));

Given than everything in here is fpscr related, perhaps "store_mask" is a 
better name?

Also, this is the second time we're modifying the new do_mffsc function.  Does it actually 
make more sense to reverse the order of these patches so that MFFSCRN comes first, as that 
is the one that takes the most complex form of do_mffsc.

Alternately, does it really make sense to try to do everything in one function, with 4 
extra parameters that turn out to be very specific to single instructions?  E.g. the t1 
parameter would probably be better implemented with a deposit of the RN field, rather than 
separate ANDs and OR.


r~