| Message ID | 20230227054233.390271-20-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tcg: Remove tcg_const_* | expand |
> -----Original Message----- > From: Richard Henderson <richard.henderson@linaro.org> > Sent: Sunday, February 26, 2023 10:42 PM > To: qemu-devel@nongnu.org > Cc: qemu-arm@nongnu.org; qemu-ppc@nongnu.org; qemu- > riscv@nongnu.org; qemu-s390x@nongnu.org; jcmvbkbc@gmail.com; > kbastian@mail.uni-paderborn.de; ysato@users.sourceforge.jp; > gaosong@loongson.cn; jiaxun.yang@flygoat.com; Taylor Simpson > <tsimpson@quicinc.com>; ale@rev.ng; mrolnik@gmail.com; > edgar.iglesias@gmail.com > Subject: [PATCH 19/70] target/hexagon/idef-parser: Use gen_constant for > gen_extend_tcg_width_op > > We already have a temporary, res, which we can use for the intermediate > shift result. Simplify the constant to -1 instead of 0xf*f. > This was the last use of gen_tmp_value, so remove it. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/hexagon/idef-parser/parser-helpers.c | 30 +++------------------ > 1 file changed, 3 insertions(+), 27 deletions(-) > > diff --git a/target/hexagon/idef-parser/parser-helpers.c > b/target/hexagon/idef-parser/parser-helpers.c > index c0e6f2190c..e1a55412c8 100644 > --- a/target/hexagon/idef-parser/parser-helpers.c > +++ b/target/hexagon/idef-parser/parser-helpers.c > @@ -1120,15 +1100,11 @@ static > HexValue gen_extend_tcg_width_op(Context *c, > OUT(c, locp, "tcg_gen_subfi_i", &dst_width); > OUT(c, locp, "(", &shift, ", ", &dst_width, ", ", &src_width_m, ");\n"); > if (signedness == UNSIGNED) { > - const char *mask_str = (dst_width == 32) > - ? "0xffffffff" > - : "0xffffffffffffffff"; > - HexValue mask = gen_tmp_value(c, locp, mask_str, > - dst_width, UNSIGNED); > + HexValue mask = gen_constant(c, locp, "-1", dst_width, > + UNSIGNED); > OUT(c, locp, "tcg_gen_shr_i", &dst_width, "(", > - &mask, ", ", &mask, ", ", &shift, ");\n"); > + &res, ", ", &mask, ", ", &shift, ");\n"); > OUT(c, locp, "tcg_gen_and_i", &dst_width, "(", > - &res, ", ", value, ", ", &mask, ");\n"); > + &res, ", ", &res, ", ", value, ");\n"); What's the advantage of putting the result of the tcg_gen_shr into res instead of mask? Is there something in TCG code generation that takes advantage? > } else { > OUT(c, locp, "tcg_gen_shl_i", &dst_width, "(", > &res, ", ", value, ", ", &shift, ");\n");
On 2/27/23 11:55, Taylor Simpson wrote: >> - HexValue mask = gen_tmp_value(c, locp, mask_str, >> - dst_width, UNSIGNED); >> + HexValue mask = gen_constant(c, locp, "-1", dst_width, >> + UNSIGNED); >> OUT(c, locp, "tcg_gen_shr_i", &dst_width, "(", >> - &mask, ", ", &mask, ", ", &shift, ");\n"); >> + &res, ", ", &mask, ", ", &shift, ");\n"); >> OUT(c, locp, "tcg_gen_and_i", &dst_width, "(", >> - &res, ", ", value, ", ", &mask, ");\n"); >> + &res, ", ", &res, ", ", value, ");\n"); > > What's the advantage of putting the result of the tcg_gen_shr into res instead of mask? Is there something in TCG code generation that takes advantage? With this patch, mask is read-only, so a write to it is illegal. r~
> -----Original Message----- > From: Richard Henderson <richard.henderson@linaro.org> > Sent: Monday, February 27, 2023 3:01 PM > To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org > Cc: qemu-arm@nongnu.org; qemu-ppc@nongnu.org; qemu- > riscv@nongnu.org; qemu-s390x@nongnu.org; jcmvbkbc@gmail.com; > kbastian@mail.uni-paderborn.de; ysato@users.sourceforge.jp; > gaosong@loongson.cn; jiaxun.yang@flygoat.com; ale@rev.ng; > mrolnik@gmail.com; edgar.iglesias@gmail.com > Subject: Re: [PATCH 19/70] target/hexagon/idef-parser: Use gen_constant > for gen_extend_tcg_width_op > > On 2/27/23 11:55, Taylor Simpson wrote: > >> - HexValue mask = gen_tmp_value(c, locp, mask_str, > >> - dst_width, UNSIGNED); > >> + HexValue mask = gen_constant(c, locp, "-1", dst_width, > >> + UNSIGNED); > >> OUT(c, locp, "tcg_gen_shr_i", &dst_width, "(", > >> - &mask, ", ", &mask, ", ", &shift, ");\n"); > >> + &res, ", ", &mask, ", ", &shift, ");\n"); > >> OUT(c, locp, "tcg_gen_and_i", &dst_width, "(", > >> - &res, ", ", value, ", ", &mask, ");\n"); > >> + &res, ", ", &res, ", ", value, ");\n"); > > > > What's the advantage of putting the result of the tcg_gen_shr into res > instead of mask? Is there something in TCG code generation that takes > advantage? > > With this patch, mask is read-only, so a write to it is illegal. I see. Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
diff --git a/target/hexagon/idef-parser/parser-helpers.c b/target/hexagon/idef-parser/parser-helpers.c index c0e6f2190c..e1a55412c8 100644 --- a/target/hexagon/idef-parser/parser-helpers.c +++ b/target/hexagon/idef-parser/parser-helpers.c @@ -305,26 +305,6 @@ HexValue gen_tmp(Context *c, return rvalue; } -static HexValue gen_tmp_value(Context *c, - YYLTYPE *locp, - const char *value, - unsigned bit_width, - HexSignedness signedness) -{ - HexValue rvalue; - assert(bit_width == 32 || bit_width == 64); - memset(&rvalue, 0, sizeof(HexValue)); - rvalue.type = TEMP; - rvalue.bit_width = bit_width; - rvalue.signedness = signedness; - rvalue.is_dotnew = false; - rvalue.tmp.index = c->inst.tmp_count; - OUT(c, locp, "TCGv_i", &bit_width, " tmp_", &c->inst.tmp_count, - " = tcg_const_i", &bit_width, "(", value, ");\n"); - c->inst.tmp_count++; - return rvalue; -} - static HexValue gen_constant_from_imm(Context *c, YYLTYPE *locp, HexValue *value) @@ -1120,15 +1100,11 @@ static HexValue gen_extend_tcg_width_op(Context *c, OUT(c, locp, "tcg_gen_subfi_i", &dst_width); OUT(c, locp, "(", &shift, ", ", &dst_width, ", ", &src_width_m, ");\n"); if (signedness == UNSIGNED) { - const char *mask_str = (dst_width == 32) - ? "0xffffffff" - : "0xffffffffffffffff"; - HexValue mask = gen_tmp_value(c, locp, mask_str, - dst_width, UNSIGNED); + HexValue mask = gen_constant(c, locp, "-1", dst_width, UNSIGNED); OUT(c, locp, "tcg_gen_shr_i", &dst_width, "(", - &mask, ", ", &mask, ", ", &shift, ");\n"); + &res, ", ", &mask, ", ", &shift, ");\n"); OUT(c, locp, "tcg_gen_and_i", &dst_width, "(", - &res, ", ", value, ", ", &mask, ");\n"); + &res, ", ", &res, ", ", value, ");\n"); } else { OUT(c, locp, "tcg_gen_shl_i", &dst_width, "(", &res, ", ", value, ", ", &shift, ");\n");
We already have a temporary, res, which we can use for the intermediate shift result. Simplify the constant to -1 instead of 0xf*f. This was the last use of gen_tmp_value, so remove it. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/hexagon/idef-parser/parser-helpers.c | 30 +++------------------ 1 file changed, 3 insertions(+), 27 deletions(-)