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

[Chinmay Rath <rathc@linux.vnet.ibm.com>]

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~