| Message ID | 20220927064838.3570928-3-gaosong@loongson.cn |
|---|---|
| State | New |
| Headers | show |
| Series | Fix some loongarch tcg bugs | expand |
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)
在 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)
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~ 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)
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)
Signed-off-by: Song Gao <gaosong@loongson.cn> --- target/loongarch/insn_trans/trans_bit.c.inc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)