Re: [PATCH v2 2/4] target/ppc: Move VSX vector with length storage access insns to decodetree.

[Chinmay Rath]

Hi Richard,

On 6/17/24 00:43, Richard Henderson wrote:
> On 6/13/24 02:33, Chinmay Rath wrote:
> > +/* EA <- (ra == 0) ? 0 : GPR[ra] */
> > +static TCGv do_ea_calc_ra(DisasContext *ctx, int ra)
> > +{
> > +    TCGv EA;
> > +    if (!ra) {
> > +        EA = tcg_constant_tl(0);
> > +        return EA;
> > +    }
> > +    EA = tcg_temp_new();
> > +    if (NARROW_MODE(ctx)) {
> > +        tcg_gen_ext32u_tl(EA, cpu_gpr[ra]);
> > +    } else {
> > +        tcg_gen_mov_tl(EA, cpu_gpr[ra]);
>
> Why are you making a copy, rather than just returning cpu_gpr[ra]?
True, this tcg move is redundant. Was carried away to maintain 
uniformity with the original do_ea_calc function. My bad!

This can rather just be :
/* ea <- (ra == 0) ? 0 : GPR[ra] */
static TCGv do_ea_calc_ra(DisasContext *ctx, int ra)
{
    TCGv EA;
    if (!ra) {
        return tcg_constant_tl(0);
    }
    if (NARROW_MODE(ctx)) {
        EA = tcg_temp_new();
        tcg_gen_ext32u_tl(EA, cpu_gpr[ra]);
    } else {
        return cpu_gpr[ra];
    }
    return EA;
}

> If you need to modify the resulting EA, then you also need to make a 
> copy for 0.
Hey, didn't properly get what you meant here.
Did you mean : Since I'm using a tcg_constant for 0, if the EA is to be 
modified later, this constant would be an issue, in which case, I should 
make a copy for it ??

Considering that, there are no tcg level modifications with this EA. 
However, the underlying helper method, which considers this EA as a 
target_ulong type does modify it, which I don't think should be an issue.

Please let me know if I missed something.

Thanks & Regards,
Chinmay
> r~