Re: [PATCH v2 12/22] target/loongarch: Add fixed point extra instruction translation

[Song Gao]

Hi, Richard.

On 07/23/2021 01:12 PM, Richard Henderson wrote:
> On 7/20/21 11:53 PM, Song Gao wrote:
>> +target_ulong helper_cpucfg(CPULoongArchState *env, target_ulong rj)
>> +{
>> +    target_ulong r = 0;
>> +
>> +    switch (rj) {
>> +    case 0:
>> +        r = env->CSR_MCSR0 & 0xffffffff;
>> +        break;
>> +    case 1:
>> +        r = (env->CSR_MCSR0 & 0xffffffff00000000) >> 32;
>> +        break;
> 
> Why do you represent all of these as high and low portions of a 64-bit 
> internal value, when the manual describes them as 32-bit values?
> 
This method can reduce variables on env.
> >> +/* Fixed point extra instruction translation */
>> +static bool trans_crc_w_b_w(DisasContext *ctx, arg_crc_w_b_w *a)
>> +{
>> +    TCGv t0, t1;
>> +    TCGv Rd = cpu_gpr[a->rd];
>> +    TCGv_i32 tsz = tcg_const_i32(1 << 1);
> 
> This size is wrong.  It should be 1, not 1 << 1 (2).
> > 
>> +static bool trans_crc_w_w_w(DisasContext *ctx, arg_crc_w_w_w *a)
>> +{
>> +    TCGv t0, t1;
>> +    TCGv Rd = cpu_gpr[a->rd];
>> +    TCGv_i32 tsz = tcg_const_i32(1 << 4);
> 
> Because this size most certainly should not be 16...
>>> +static bool trans_crc_w_d_w(DisasContext *ctx, arg_crc_w_d_w *a)
>> +{
>> +    TCGv t0, t1;
>> +    TCGv Rd = cpu_gpr[a->rd];
>> +    TCGv_i32 tsz = tcg_const_i32(1 << 8);
> 
> ... and this size should not be 256.  Both well larger than the 8 byte buffer 
> that you've allocated.
> 

I'm not sure about that.

> Also, you need a helper so that you don't have 8 copies of this code.
> 
OK.
>> +static bool trans_asrtle_d(DisasContext *ctx, arg_asrtle_d * a)
>> +{
>> +    TCGv t0, t1;
>> +
>> +    t0 = get_gpr(a->rj);
>> +    t1 = get_gpr(a->rk);
>> +
>> +    gen_helper_asrtle_d(cpu_env, t0, t1);
>> +
>> +    return true;
>> +}
>> +
>> +static bool trans_asrtgt_d(DisasContext *ctx, arg_asrtgt_d * a)
>> +{
>> +    TCGv t0, t1;
>> +
>> +    t0 = get_gpr(a->rj);
>> +    t1 = get_gpr(a->rk);
>> +
>> +    gen_helper_asrtgt_d(cpu_env, t0, t1);
>> +
>> +    return true;
>> +}
> 
> I'm not sure why both of these instructions are in the ISA, since
> 
>   ASRTLE X,Y <-> ASRTGT Y,X
> 
> but we certainly don't need two different helpers.
> Just swap the arguments for one of them.
>
OK.
 
>> +static bool trans_rdtimel_w(DisasContext *ctx, arg_rdtimel_w *a)
>> +{
>> +    /* Nop */
>> +    return true;
>> +}
>> +
>> +static bool trans_rdtimeh_w(DisasContext *ctx, arg_rdtimeh_w *a)
>> +{
>> +    /* Nop */
>> +    return true;
>> +}
>> +
>> +static bool trans_rdtime_d(DisasContext *ctx, arg_rdtime_d *a)
>> +{
>> +    /* Nop */
>> +    return true;
>> +}
> 
> If you don't want to implement these right now, you should at least 
> initialize the destination register to 0, or something.
> 
OK.
> 
> r~

Again ,thanks you kindly help.

Thanks
Song Gao.