[PATCH v2 0/4] Fix some loongarch tcg bugs

[PATCH v2 0/4] Fix some loongarch tcg bugs

[Song Gao]

Hi,

This series fix some bugs find from RISU test.

v2:
  -remove patch5 div if x/0 set dividend to 0.

Song Gao (4):
  target/loongarch: ftint_xxx insns set the result high 32bit 0xffffffff
  target/loongarch: bstrins.w need set dest register EXT_SIGN
  target/loongarch: Fix fnm{sub/add}_{s/d} set wrong flags
  target/loongarch: flogb_{s/d} add set float_flag_divbyzero

 target/loongarch/fpu_helper.c                 | 32 +++++++++++++------
 target/loongarch/insn_trans/trans_bit.c.inc   |  4 +--
 .../loongarch/insn_trans/trans_farith.c.inc   | 12 +++----
 3 files changed, 31 insertions(+), 17 deletions(-)

-- 
2.31.1

[PATCH v2 1/4] target/loongarch: ftint_xxx insns set the result high 32bit 0xffffffff

[Song Gao]

we just set high 32bit 0xffffffff as the other float instructions do.

Signed-off-by: Song Gao <gaosong@loongson.cn>
---
 target/loongarch/fpu_helper.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/target/loongarch/fpu_helper.c b/target/loongarch/fpu_helper.c
index 4b9637210a..1a24667eaf 100644
--- a/target/loongarch/fpu_helper.c
+++ b/target/loongarch/fpu_helper.c
@@ -518,7 +518,7 @@ uint64_t helper_frint_s(CPULoongArchState *env, uint64_t fj)
 {
     uint64_t fd;
 
-    fd = (uint64_t)(float32_round_to_int((uint32_t)fj, &env->fp_status));
+    fd = nanbox_s(float32_round_to_int((uint32_t)fj, &env->fp_status));
     update_fcsr0(env, GETPC());
     return fd;
 }
@@ -574,7 +574,7 @@ uint64_t helper_ftintrm_w_d(CPULoongArchState *env, uint64_t fj)
     FloatRoundMode old_mode = get_float_rounding_mode(&env->fp_status);
 
     set_float_rounding_mode(float_round_down, &env->fp_status);
-    fd = (uint64_t)float64_to_int32(fj, &env->fp_status);
+    fd = nanbox_s(float64_to_int32(fj, &env->fp_status));
     set_float_rounding_mode(old_mode, &env->fp_status);
 
     if (get_float_exception_flags(&env->fp_status) & (float_flag_invalid)) {
@@ -592,7 +592,7 @@ uint64_t helper_ftintrm_w_s(CPULoongArchState *env, uint64_t fj)
     FloatRoundMode old_mode = get_float_rounding_mode(&env->fp_status);
 
     set_float_rounding_mode(float_round_down, &env->fp_status);
-    fd = (uint64_t)float32_to_int32((uint32_t)fj, &env->fp_status);
+    fd = nanbox_s(float32_to_int32((uint32_t)fj, &env->fp_status));
     set_float_rounding_mode(old_mode, &env->fp_status);
 
     if (get_float_exception_flags(&env->fp_status) & (float_flag_invalid)) {
@@ -646,7 +646,7 @@ uint64_t helper_ftintrp_w_d(CPULoongArchState *env, uint64_t fj)
     FloatRoundMode old_mode = get_float_rounding_mode(&env->fp_status);
 
     set_float_rounding_mode(float_round_up, &env->fp_status);
-    fd = (uint64_t)float64_to_int32(fj, &env->fp_status);
+    fd = nanbox_s(float64_to_int32(fj, &env->fp_status));
     set_float_rounding_mode(old_mode, &env->fp_status);
 
     if (get_float_exception_flags(&env->fp_status) & (float_flag_invalid)) {
@@ -664,7 +664,7 @@ uint64_t helper_ftintrp_w_s(CPULoongArchState *env, uint64_t fj)
     FloatRoundMode old_mode = get_float_rounding_mode(&env->fp_status);
 
     set_float_rounding_mode(float_round_up, &env->fp_status);
-    fd = (uint64_t)float32_to_int32((uint32_t)fj, &env->fp_status);
+    fd = nanbox_s(float32_to_int32((uint32_t)fj, &env->fp_status));
     set_float_rounding_mode(old_mode, &env->fp_status);
 
     if (get_float_exception_flags(&env->fp_status) & (float_flag_invalid)) {
@@ -715,7 +715,7 @@ uint64_t helper_ftintrz_w_d(CPULoongArchState *env, uint64_t fj)
     uint64_t fd;
     FloatRoundMode old_mode = get_float_rounding_mode(&env->fp_status);
 
-    fd = (uint64_t)float64_to_int32_round_to_zero(fj, &env->fp_status);
+    fd = nanbox_s(float64_to_int32_round_to_zero(fj, &env->fp_status));
     set_float_rounding_mode(old_mode, &env->fp_status);
 
     if (get_float_exception_flags(&env->fp_status) & (float_flag_invalid)) {
@@ -786,7 +786,7 @@ uint64_t helper_ftintrne_w_d(CPULoongArchState *env, uint64_t fj)
     FloatRoundMode old_mode = get_float_rounding_mode(&env->fp_status);
 
     set_float_rounding_mode(float_round_nearest_even, &env->fp_status);
-    fd = (uint64_t)float64_to_int32(fj, &env->fp_status);
+    fd = nanbox_s(float64_to_int32(fj, &env->fp_status));
     set_float_rounding_mode(old_mode, &env->fp_status);
 
     if (get_float_exception_flags(&env->fp_status) & (float_flag_invalid)) {
@@ -848,7 +848,7 @@ uint64_t helper_ftint_w_s(CPULoongArchState *env, uint64_t fj)
 {
     uint64_t fd;
 
-    fd = (uint64_t)float32_to_int32((uint32_t)fj, &env->fp_status);
+    fd = nanbox_s(float32_to_int32((uint32_t)fj, &env->fp_status));
     if (get_float_exception_flags(&env->fp_status) & (float_flag_invalid)) {
         if (float32_is_any_nan((uint32_t)fj)) {
             fd = 0;
@@ -862,7 +862,7 @@ uint64_t helper_ftint_w_d(CPULoongArchState *env, uint64_t fj)
 {
     uint64_t fd;
 
-    fd = (uint64_t)float64_to_int32(fj, &env->fp_status);
+    fd = nanbox_s(float64_to_int32(fj, &env->fp_status));
     if (get_float_exception_flags(&env->fp_status) & (float_flag_invalid)) {
         if (float64_is_any_nan(fj)) {
             fd = 0;
-- 
2.31.1

[PATCH v2 2/4] target/loongarch: bstrins.w need set dest register EXT_SIGN

[Song Gao]

Signed-off-by: Song Gao <gaosong@loongson.cn>
---
 target/loongarch/insn_trans/trans_bit.c.inc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/loongarch/insn_trans/trans_bit.c.inc b/target/loongarch/insn_trans/trans_bit.c.inc
index 9337714ec4..33e94878fd 100644
--- a/target/loongarch/insn_trans/trans_bit.c.inc
+++ b/target/loongarch/insn_trans/trans_bit.c.inc
@@ -37,7 +37,7 @@ static bool gen_rr_ms_ls(DisasContext *ctx, arg_rr_ms_ls *a,
                          DisasExtend src_ext, DisasExtend dst_ext,
                          void (*func)(TCGv, TCGv, unsigned int, unsigned int))
 {
-    TCGv dest = gpr_dst(ctx, a->rd, dst_ext);
+    TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
     TCGv src1 = gpr_src(ctx, a->rj, src_ext);
 
     if (a->ls > a->ms) {
@@ -206,7 +206,7 @@ TRANS(maskeqz, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_maskeqz)
 TRANS(masknez, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_masknez)
 TRANS(bytepick_w, gen_rrr_sa, EXT_NONE, EXT_NONE, gen_bytepick_w)
 TRANS(bytepick_d, gen_rrr_sa, EXT_NONE, EXT_NONE, gen_bytepick_d)
-TRANS(bstrins_w, gen_rr_ms_ls, EXT_NONE, EXT_NONE, gen_bstrins)
+TRANS(bstrins_w, gen_rr_ms_ls, EXT_NONE, EXT_SIGN, gen_bstrins)
 TRANS(bstrins_d, gen_rr_ms_ls, EXT_NONE, EXT_NONE, gen_bstrins)
 TRANS(bstrpick_w, gen_rr_ms_ls, EXT_NONE, EXT_SIGN, tcg_gen_extract_tl)
 TRANS(bstrpick_d, gen_rr_ms_ls, EXT_NONE, EXT_NONE, tcg_gen_extract_tl)
-- 
2.31.1

[PATCH v2 3/4] target/loongarch: Fix fnm{sub/add}_{s/d} set wrong flags

[Song Gao]

Signed-off-by: Song Gao <gaosong@loongson.cn>
---
 target/loongarch/insn_trans/trans_farith.c.inc | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/target/loongarch/insn_trans/trans_farith.c.inc b/target/loongarch/insn_trans/trans_farith.c.inc
index 65ad2ffab8..7bb3f41aee 100644
--- a/target/loongarch/insn_trans/trans_farith.c.inc
+++ b/target/loongarch/insn_trans/trans_farith.c.inc
@@ -97,9 +97,9 @@ TRANS(fmadd_s, gen_muladd, gen_helper_fmuladd_s, 0)
 TRANS(fmadd_d, gen_muladd, gen_helper_fmuladd_d, 0)
 TRANS(fmsub_s, gen_muladd, gen_helper_fmuladd_s, float_muladd_negate_c)
 TRANS(fmsub_d, gen_muladd, gen_helper_fmuladd_d, float_muladd_negate_c)
-TRANS(fnmadd_s, gen_muladd, gen_helper_fmuladd_s,
-      float_muladd_negate_product | float_muladd_negate_c)
-TRANS(fnmadd_d, gen_muladd, gen_helper_fmuladd_d,
-      float_muladd_negate_product | float_muladd_negate_c)
-TRANS(fnmsub_s, gen_muladd, gen_helper_fmuladd_s, float_muladd_negate_product)
-TRANS(fnmsub_d, gen_muladd, gen_helper_fmuladd_d, float_muladd_negate_product)
+TRANS(fnmadd_s, gen_muladd, gen_helper_fmuladd_s, float_muladd_negate_result)
+TRANS(fnmadd_d, gen_muladd, gen_helper_fmuladd_d, float_muladd_negate_result)
+TRANS(fnmsub_s, gen_muladd, gen_helper_fmuladd_s,
+      float_muladd_negate_c | float_muladd_negate_result)
+TRANS(fnmsub_d, gen_muladd, gen_helper_fmuladd_d,
+      float_muladd_negate_c | float_muladd_negate_result)
-- 
2.31.1

[PATCH v2 4/4] target/loongarch: flogb_{s/d} add set float_flag_divbyzero

[Song Gao]

if fj ==0 or fj == INT32_MIN/INT64_MIN, LoongArch host set fcsr cause exception FP_DIV0,
So we need set exception flags float_flagdivbyzero if fj ==0.

Signed-off-by: Song Gao <gaosong@loongson.cn>
---
 target/loongarch/fpu_helper.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/target/loongarch/fpu_helper.c b/target/loongarch/fpu_helper.c
index 1a24667eaf..d40e608bb4 100644
--- a/target/loongarch/fpu_helper.c
+++ b/target/loongarch/fpu_helper.c
@@ -322,6 +322,13 @@ uint64_t helper_flogb_s(CPULoongArchState *env, uint64_t fj)
     fp = float32_log2((uint32_t)fj, status);
     fd = nanbox_s(float32_round_to_int(fp, status));
     set_float_rounding_mode(old_mode, status);
+    /*
+     * LoongArch host if fj == 0 or INT32_MIN , set the fcsr cause FP_DIV0
+     * so we need set exception flags float_flag_divbyzero.
+     */
+    if (((uint32_t)fj == 0) | ((uint32_t)fj == INT32_MIN)) {
+        set_float_exception_flags(float_flag_divbyzero, status);
+    }
     update_fcsr0_mask(env, GETPC(), float_flag_inexact);
     return fd;
 }
@@ -336,6 +343,13 @@ uint64_t helper_flogb_d(CPULoongArchState *env, uint64_t fj)
     fd = float64_log2(fj, status);
     fd = float64_round_to_int(fd, status);
     set_float_rounding_mode(old_mode, status);
+    /*
+     * LoongArch host if fj == 0 or INT64_MIN , set the fcsr cause FP_DIV0
+     * so we need set exception flags float_flag_divbyzero.
+     */
+    if ((fj == 0) | (fj == INT64_MIN)) {
+        set_float_exception_flags(float_flag_divbyzero, status);
+    }
     update_fcsr0_mask(env, GETPC(), float_flag_inexact);
     return fd;
 }
-- 
2.31.1

Re: [PATCH v2 2/4] target/loongarch: bstrins.w need set dest register EXT_SIGN

[Richard Henderson]

On 9/26/22 23:48, Song Gao wrote:
> Signed-off-by: Song Gao <gaosong@loongson.cn>
> ---
>   target/loongarch/insn_trans/trans_bit.c.inc | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/loongarch/insn_trans/trans_bit.c.inc b/target/loongarch/insn_trans/trans_bit.c.inc
> index 9337714ec4..33e94878fd 100644
> --- a/target/loongarch/insn_trans/trans_bit.c.inc
> +++ b/target/loongarch/insn_trans/trans_bit.c.inc
> @@ -37,7 +37,7 @@ static bool gen_rr_ms_ls(DisasContext *ctx, arg_rr_ms_ls *a,
>                            DisasExtend src_ext, DisasExtend dst_ext,
>                            void (*func)(TCGv, TCGv, unsigned int, unsigned int))
>   {
> -    TCGv dest = gpr_dst(ctx, a->rd, dst_ext);
> +    TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
>       TCGv src1 = gpr_src(ctx, a->rj, src_ext);
>   
>       if (a->ls > a->ms) {
> @@ -206,7 +206,7 @@ TRANS(maskeqz, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_maskeqz)
>   TRANS(masknez, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_masknez)
>   TRANS(bytepick_w, gen_rrr_sa, EXT_NONE, EXT_NONE, gen_bytepick_w)
>   TRANS(bytepick_d, gen_rrr_sa, EXT_NONE, EXT_NONE, gen_bytepick_d)
> -TRANS(bstrins_w, gen_rr_ms_ls, EXT_NONE, EXT_NONE, gen_bstrins)
> +TRANS(bstrins_w, gen_rr_ms_ls, EXT_NONE, EXT_SIGN, gen_bstrins)

These two hunks do opposite things: change dst_ext from NONE to SIGN, and then ignore the 
change.

I assume the first hunk is in fact in error.


r~


>   TRANS(bstrins_d, gen_rr_ms_ls, EXT_NONE, EXT_NONE, gen_bstrins)
>   TRANS(bstrpick_w, gen_rr_ms_ls, EXT_NONE, EXT_SIGN, tcg_gen_extract_tl)
>   TRANS(bstrpick_d, gen_rr_ms_ls, EXT_NONE, EXT_NONE, tcg_gen_extract_tl)

Re: [PATCH v2 1/4] target/loongarch: ftint_xxx insns set the result high 32bit 0xffffffff

[Richard Henderson]

On 9/26/22 23:48, Song Gao wrote:
> we just set high 32bit 0xffffffff as the other float instructions do.
> 
> Signed-off-by: Song Gao<gaosong@loongson.cn>
> ---
>   target/loongarch/fpu_helper.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)

But the result in these cases is an integer, not a (single-precision) float.
Is this really what hardware does?


r~

Re: [PATCH v2 3/4] target/loongarch: Fix fnm{sub/add}_{s/d} set wrong flags

[Richard Henderson]

On 9/26/22 23:48, Song Gao wrote:
> Signed-off-by: Song Gao<gaosong@loongson.cn>
> ---
>   target/loongarch/insn_trans/trans_farith.c.inc | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 4/4] target/loongarch: flogb_{s/d} add set float_flag_divbyzero

[Richard Henderson]

On 9/26/22 23:48, Song Gao wrote:
> if fj ==0 or fj == INT32_MIN/INT64_MIN, LoongArch host set fcsr cause exception FP_DIV0,
> So we need set exception flags float_flagdivbyzero if fj ==0.

You are correct that ieee754 says that logB(0) should raise divbyzero.
This should be fixed in softfloat-parts.c.inc, not here, within

         case float_class_zero:

             /* log2(0) = -inf */

             a->cls = float_class_inf;

             a->sign = 1;

             return;



r~

Re: [PATCH v2 1/4] target/loongarch: ftint_xxx insns set the result high 32bit 0xffffffff

[gaosong]

[-- Attachment #1: Type: text/plain, Size: 958 bytes --]


在 2022/9/28 下午11:14, Richard Henderson 写道:
> On 9/26/22 23:48, Song Gao wrote:
>> we just set high 32bit 0xffffffff as the other float instructions do.
>>
>> Signed-off-by: Song Gao<gaosong@loongson.cn>
>> ---
>>   target/loongarch/fpu_helper.c | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>
> But the result in these cases is an integer, not a (single-precision) 
> float.
> Is this really what hardware does?
>
The high 32bit value is not fixed  as the manual 3.1.3.1 said:
     ' When the floating-point register records a single-precision 
floating-point number or word integer, the data
     always appears in the [31:0] bits of the floating-point register, 
at this time the [63:32] bits of the
     floating-point register can be any value.'
I do this  just used for RISU test compare these instructions result value.
As the RISU patches not reviewed,  I can drop this patch.

Thanks.
Song Gao

[-- Attachment #2: Type: text/html, Size: 1747 bytes --]

Re: [PATCH v2 2/4] target/loongarch: bstrins.w need set dest register EXT_SIGN

[gaosong]

在 2022/9/28 下午11:13, Richard Henderson 写道:
> On 9/26/22 23:48, Song Gao wrote:
>> Signed-off-by: Song Gao <gaosong@loongson.cn>
>> ---
>>   target/loongarch/insn_trans/trans_bit.c.inc | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/loongarch/insn_trans/trans_bit.c.inc 
>> b/target/loongarch/insn_trans/trans_bit.c.inc
>> index 9337714ec4..33e94878fd 100644
>> --- a/target/loongarch/insn_trans/trans_bit.c.inc
>> +++ b/target/loongarch/insn_trans/trans_bit.c.inc
>> @@ -37,7 +37,7 @@ static bool gen_rr_ms_ls(DisasContext *ctx, 
>> arg_rr_ms_ls *a,
>>                            DisasExtend src_ext, DisasExtend dst_ext,
>>                             void (*func)(TCGv, TCGv, unsigned int, 
>> unsigned int))
>>   {
>> -    TCGv dest = gpr_dst(ctx, a->rd, dst_ext);
>> +    TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
>>       TCGv src1 = gpr_src(ctx, a->rj, src_ext);
>>         if (a->ls > a->ms) {
>> @@ -206,7 +206,7 @@ TRANS(maskeqz, gen_rrr, EXT_NONE, EXT_NONE, 
>> EXT_NONE, gen_maskeqz)
>>   TRANS(masknez, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_masknez)
>>   TRANS(bytepick_w, gen_rrr_sa, EXT_NONE, EXT_NONE, gen_bytepick_w)
>>   TRANS(bytepick_d, gen_rrr_sa, EXT_NONE, EXT_NONE, gen_bytepick_d)
>> -TRANS(bstrins_w, gen_rr_ms_ls, EXT_NONE, EXT_NONE, gen_bstrins)
>> +TRANS(bstrins_w, gen_rr_ms_ls, EXT_NONE, EXT_SIGN, gen_bstrins)
>
> These two hunks do opposite things: change dst_ext from NONE to SIGN, 
> and then ignore the change.
>
> I assume the first hunk is in fact in error.
>
rd is also a src register,    rd should be src_dst.

     TCGv dest = gpr_src(ctx, a->rd, src_dst);   what about this?


Thanks.
Song Gao
>
> r~
>
>
>>   TRANS(bstrins_d, gen_rr_ms_ls, EXT_NONE, EXT_NONE, gen_bstrins)
>>   TRANS(bstrpick_w, gen_rr_ms_ls, EXT_NONE, EXT_SIGN, 
>> tcg_gen_extract_tl)
>>   TRANS(bstrpick_d, gen_rr_ms_ls, EXT_NONE, EXT_NONE, 
>> tcg_gen_extract_tl)

Re: [PATCH v2 4/4] target/loongarch: flogb_{s/d} add set float_flag_divbyzero

[gaosong]

在 2022/9/28 下午11:24, Richard Henderson 写道:
> On 9/26/22 23:48, Song Gao wrote:
>> if fj ==0 or fj == INT32_MIN/INT64_MIN, LoongArch host set fcsr cause 
>> exception FP_DIV0,
>> So we need set exception flags float_flagdivbyzero if fj ==0.
>
> You are correct that ieee754 says that logB(0) should raise divbyzero.
> This should be fixed in softfloat-parts.c.inc, not here, within
>
>         case float_class_zero:
>
>             /* log2(0) = -inf */
>
>             a->cls = float_class_inf;
>
>             a->sign = 1;
>
>             return;
>
>
Ok , I will correct it on v3.

Thanks.
Song Gao
>
> r~

Re: [PATCH v2 2/4] target/loongarch: bstrins.w need set dest register EXT_SIGN

[Richard Henderson]

[-- Attachment #1: Type: text/plain, Size: 2045 bytes --]

On 9/29/22 00:27, gaosong wrote:
> 
> 在 2022/9/28 下午11:13, Richard Henderson 写道:
>> On 9/26/22 23:48, Song Gao wrote:
>>> Signed-off-by: Song Gao <gaosong@loongson.cn>
>>> ---
>>>   target/loongarch/insn_trans/trans_bit.c.inc | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target/loongarch/insn_trans/trans_bit.c.inc 
>>> b/target/loongarch/insn_trans/trans_bit.c.inc
>>> index 9337714ec4..33e94878fd 100644
>>> --- a/target/loongarch/insn_trans/trans_bit.c.inc
>>> +++ b/target/loongarch/insn_trans/trans_bit.c.inc
>>> @@ -37,7 +37,7 @@ static bool gen_rr_ms_ls(DisasContext *ctx, arg_rr_ms_ls *a,
>>>                            DisasExtend src_ext, DisasExtend dst_ext,
>>>                             void (*func)(TCGv, TCGv, unsigned int, unsigned int))
>>>   {
>>> -    TCGv dest = gpr_dst(ctx, a->rd, dst_ext);
>>> +    TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
>>>       TCGv src1 = gpr_src(ctx, a->rj, src_ext);
>>>         if (a->ls > a->ms) {
>>> @@ -206,7 +206,7 @@ TRANS(maskeqz, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_maskeqz)
>>>   TRANS(masknez, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_masknez)
>>>   TRANS(bytepick_w, gen_rrr_sa, EXT_NONE, EXT_NONE, gen_bytepick_w)
>>>   TRANS(bytepick_d, gen_rrr_sa, EXT_NONE, EXT_NONE, gen_bytepick_d)
>>> -TRANS(bstrins_w, gen_rr_ms_ls, EXT_NONE, EXT_NONE, gen_bstrins)
>>> +TRANS(bstrins_w, gen_rr_ms_ls, EXT_NONE, EXT_SIGN, gen_bstrins)
>>
>> These two hunks do opposite things: change dst_ext from NONE to SIGN, and then ignore 
>> the change.
>>
>> I assume the first hunk is in fact in error.
>>
> rd is also a src register,    rd should be src_dst.
> 
>      TCGv dest = gpr_src(ctx, a->rd, src_dst);   what about this?

Ah, I see the problem.  We're sharing a helper meant for unary operations for an 
instruction that is a binary operation with an in+out operand.

I suggest not attempting to share code between bstrins and bstrpick, like so.


r~

[-- Attachment #2: bstrins.txt --]
[-- Type: text/plain, Size: 2493 bytes --]

diff --git a/target/loongarch/insn_trans/trans_bit.c.inc b/target/loongarch/insn_trans/trans_bit.c.inc
index 9337714ec4..b01e4aeb23 100644
--- a/target/loongarch/insn_trans/trans_bit.c.inc
+++ b/target/loongarch/insn_trans/trans_bit.c.inc
@@ -27,26 +27,34 @@ static void gen_bytepick_d(TCGv dest, TCGv src1, TCGv src2, target_long sa)
     tcg_gen_extract2_i64(dest, src1, src2, (64 - sa * 8));
 }
 
-static void gen_bstrins(TCGv dest, TCGv src1,
-                        unsigned int ls, unsigned int len)
+static bool gen_bstrins(DisasContext *ctx, arg_rr_ms_ls *a,
+                        DisasExtend dst_ext)
 {
-    tcg_gen_deposit_tl(dest, dest, src1, ls, len);
-}
-
-static bool gen_rr_ms_ls(DisasContext *ctx, arg_rr_ms_ls *a,
-                         DisasExtend src_ext, DisasExtend dst_ext,
-                         void (*func)(TCGv, TCGv, unsigned int, unsigned int))
-{
-    TCGv dest = gpr_dst(ctx, a->rd, dst_ext);
-    TCGv src1 = gpr_src(ctx, a->rj, src_ext);
+    TCGv src1 = gpr_src(ctx, a->rd, EXT_NONE);
+    TCGv src2 = gpr_src(ctx, a->rj, EXT_NONE);
+    TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
 
     if (a->ls > a->ms) {
         return false;
     }
 
-    func(dest, src1, a->ls, a->ms - a->ls + 1);
+    tcg_gen_deposit_tl(dest, src1, src2, a->ls, a->ms - a->ls + 1);
     gen_set_gpr(a->rd, dest, dst_ext);
+    return true;
+}
 
+static bool gen_bstrpick(DisasContext *ctx, arg_rr_ms_ls *a,
+                         DisasExtend dst_ext)
+{
+    TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
+    TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE);
+
+    if (a->ls > a->ms) {
+        return false;
+    }
+
+    tcg_gen_extract_tl(dest, src1, a->ls, a->ms - a->ls + 1);
+    gen_set_gpr(a->rd, dest, dst_ext);
     return true;
 }
 
@@ -206,7 +214,7 @@ TRANS(maskeqz, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_maskeqz)
 TRANS(masknez, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_masknez)
 TRANS(bytepick_w, gen_rrr_sa, EXT_NONE, EXT_NONE, gen_bytepick_w)
 TRANS(bytepick_d, gen_rrr_sa, EXT_NONE, EXT_NONE, gen_bytepick_d)
-TRANS(bstrins_w, gen_rr_ms_ls, EXT_NONE, EXT_NONE, gen_bstrins)
-TRANS(bstrins_d, gen_rr_ms_ls, EXT_NONE, EXT_NONE, gen_bstrins)
-TRANS(bstrpick_w, gen_rr_ms_ls, EXT_NONE, EXT_SIGN, tcg_gen_extract_tl)
-TRANS(bstrpick_d, gen_rr_ms_ls, EXT_NONE, EXT_NONE, tcg_gen_extract_tl)
+TRANS(bstrins_w, gen_bstrins, EXT_SIGN)
+TRANS(bstrins_d, gen_bstrins, EXT_NONE)
+TRANS(bstrpick_w, gen_bstrpick, EXT_SIGN)
+TRANS(bstrpick_d, gen_bstrpick, EXT_NONE)