Re: [PATCH v11 04/26] target/loongarch: Add fixed point arithmetic instruction translation
On 11/22/21 9:23 AM, gaosong wrote: On 2021/11/20 下午4:56, Richard Henderson wrote: On 11/20/21 9:52 AM, gaosong wrote: You're right, gen_set_gpr not need EXT_NONE at all, and we need not condition around gen_set_gpr. I think that if we know the dst_ext is EXT_NONE, we do't need gen_set_gpr. But that assumes that gpr_dst did not return a temporary. I think it's cleaner to assume that gen_set_gpr is needed. Does this mean that we gen_set_gpr where used gpr_dst, and gen_set_gpr need EXT_NONE? Such as gpr_dst in trans_atomic.c.inc/trans_memory.c.inc, should we need gen_set_gpr? Yes. r~
Re: [PATCH v11 04/26] target/loongarch: Add fixed point arithmetic instruction translation
Hi Richard, Sorry for the late reply. On 2021/11/20 下午4:56, Richard Henderson wrote: On 11/20/21 9:52 AM, gaosong wrote: You're right, gen_set_gpr not need EXT_NONE at all, and we need not condition around gen_set_gpr. I think that if we know the dst_ext is EXT_NONE, we do't need gen_set_gpr. But that assumes that gpr_dst did not return a temporary. I think it's cleaner to assume that gen_set_gpr is needed. Does this mean that we gen_set_gpr where used gpr_dst, and gen_set_gpr need EXT_NONE? Such as gpr_dst in trans_atomic.c.inc/trans_memory.c.inc, should we need gen_set_gpr? Thanks Song Gao r~
Re: [PATCH v11 04/26] target/loongarch: Add fixed point arithmetic instruction translation
On 11/20/21 9:52 AM, gaosong wrote: You're right, gen_set_gpr not need EXT_NONE at all, and we need not condition around gen_set_gpr. I think that if we know the dst_ext is EXT_NONE, we do't need gen_set_gpr. But that assumes that gpr_dst did not return a temporary. I think it's cleaner to assume that gen_set_gpr is needed. r~
Re: [PATCH v11 04/26] target/loongarch: Add fixed point arithmetic instruction translation
Hi Richard,
On 2021/11/20 下午3:17, Richard Henderson wrote:
On 11/19/21 7:13 AM, Song Gao wrote:
+static bool gen_rrr(DisasContext *ctx, arg_rrr *a,
+ DisasExtend src1_ext, DisasExtend src2_ext,
+ DisasExtend dst_ext, void (*func)(TCGv, TCGv,
TCGv))
+{
+ TCGv dest = gpr_dst(ctx, a->rd, dst_ext);
+ TCGv src1 = gpr_src(ctx, a->rj, src1_ext);
+ TCGv src2 = gpr_src(ctx, a->rk, src2_ext);
+
+ func(dest, src1, src2);
+
+ /* dst_ext is EXT_NONE and input is dest, We don't run
gen_set_gpr. */
+ if (dst_ext) {
+ gen_set_gpr(a->rd, dest, dst_ext);
+ }
Why the (incomplete) condition around gen_set_gpr?
I think it's a bug to not name EXT_NONE in the test (just because
EXT_NONE == 0 now...), but I also think you should not have added the
test at all. We will not generate any code in the end within
gen_set_gpr, but it allows the routines to be self-contained. You
shouldn't assume what gpr_dst returned.
You're right, gen_set_gpr not need EXT_NONE at all, and we need not condition
around gen_set_gpr.
I think that if we know the dst_ext is EXT_NONE, we do't need gen_set_gpr.
I'll correct them on v12.
Thanks
Song Gao
r~
Re: [PATCH v11 04/26] target/loongarch: Add fixed point arithmetic instruction translation
On 11/19/21 7:13 AM, Song Gao wrote:
+static bool gen_rrr(DisasContext *ctx, arg_rrr *a,
+DisasExtend src1_ext, DisasExtend src2_ext,
+DisasExtend dst_ext, void (*func)(TCGv, TCGv, TCGv))
+{
+TCGv dest = gpr_dst(ctx, a->rd, dst_ext);
+TCGv src1 = gpr_src(ctx, a->rj, src1_ext);
+TCGv src2 = gpr_src(ctx, a->rk, src2_ext);
+
+func(dest, src1, src2);
+
+/* dst_ext is EXT_NONE and input is dest, We don't run gen_set_gpr. */
+if (dst_ext) {
+gen_set_gpr(a->rd, dest, dst_ext);
+}
Why the (incomplete) condition around gen_set_gpr?
I think it's a bug to not name EXT_NONE in the test (just because EXT_NONE == 0 now...),
but I also think you should not have added the test at all. We will not generate any code
in the end within gen_set_gpr, but it allows the routines to be self-contained. You
shouldn't assume what gpr_dst returned.
r~
Re: [PATCH v11 04/26] target/loongarch: Add fixed point arithmetic instruction translation
On 11/19/21 7:13 AM, Song Gao wrote:
+static void gen_mulh_w(TCGv dest, TCGv src1, TCGv src2)
+{
+tcg_gen_mul_i64(dest, src1, src2);
+tcg_gen_sari_i64(dest, dest, 32);
+}
+
+static void gen_mulh_wu(TCGv dest, TCGv src1, TCGv src2)
+{
+tcg_gen_mul_i64(dest, src1, src2);
+tcg_gen_sari_i64(dest, dest, 32);
+}
These two are the same; you only need one of them. The difference between the two insns
is in the EXT_{SIGN,ZERO} parameter that precedes these callbacks.
+static void gen_alsl_w(TCGv dest, TCGv src1, TCGv src2,
+ TCGv temp, target_long sa)
+{
+tcg_gen_shli_tl(temp, src1, sa);
+tcg_gen_add_tl(dest, temp, src2);
+}
+
+static void gen_alsl_wu(TCGv dest, TCGv src1, TCGv src2,
+TCGv temp, target_long sa)
+{
+tcg_gen_shli_tl(temp, src1, sa);
+tcg_gen_add_tl(dest, temp, src2);
+}
+
+static void gen_alsl_d(TCGv dest, TCGv src1, TCGv src2,
+ TCGv temp, target_long sa)
+{
+tcg_gen_shli_tl(temp, src1, sa);
+tcg_gen_add_tl(dest, temp, src2);
+}
Likewise, these are identical.
r~
