diff mbox series

[19/70] target/hexagon/idef-parser: Use gen_constant for gen_extend_tcg_width_op

Message ID 20230227054233.390271-20-richard.henderson@linaro.org
State Superseded
Headers show
Series tcg: Remove tcg_const_* | expand

Commit Message

Richard Henderson Feb. 27, 2023, 5:41 a.m. UTC
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(-)

Comments

Taylor Simpson Feb. 27, 2023, 9:55 p.m. UTC | #1
> -----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");
Richard Henderson Feb. 27, 2023, 10 p.m. UTC | #2
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~
Taylor Simpson Feb. 27, 2023, 10:38 p.m. UTC | #3
> -----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 mbox series

Patch

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");