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

[Chinmay Rath]

On 6/17/24 23:27, Richard Henderson wrote:
> On 6/17/24 04:51, Chinmay Rath wrote:
> > 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]?
> > > If you need to modify the resulting EA, then you also need to make a 
> > > copy for 0.
> > Please ignore my previous response.
> > I think do_ea_calc_ra should allow modification to the resulting EA, 
> > hence below change appears more appropriate to me :
> >
> > /* EA <- (ra == 0) ? 0 : GPR[ra] */
> > static TCGv do_ea_calc_ra(DisasContext *ctx, int ra)
> > {
> >      TCGv EA = tcg_temp_new();
> >      if (!ra) {
> >          tcg_gen_movi_tl(EA, 0);
> >          return EA;
> >      }
> >      if (NARROW_MODE(ctx)) {
> >          tcg_gen_ext32u_tl(EA, cpu_gpr[ra]);
> >      } else {
> >          tcg_gen_mov_tl(EA, cpu_gpr[ra]);
> >      }
> >      return EA;
> > }
>
> If that's what's needed by the callers of do_ea_calc_ra, then yes.
> You can drop the first return EA and use else if instead.
Sure.
I shall stick to keeping EA modifiable, (even though it is not modified 
by the callers in this patch),
to allow its proper usage by (p){lx, stx}vp insns in future.

Thanks & Regards,
Chinmay
> r~