[PATCH 0/9] tcg: Use tcg_gen_[s]extract_{i32,i64,tl}

[PATCH 0/9] tcg: Use tcg_gen_[s]extract_{i32,i64,tl}

[Philippe Mathieu-Daudé]

Based-on: <20231019182921.1772928-1-richard.henderson@linaro.org>

Philippe Mathieu-Daudé (9):
  target/avr: Use tcg_gen_extract_tl
  target/cris: Use tcg_gen_extract_tl
  target/mips: Use tcg_gen_extract_i32
  target/ppc: Use tcg_gen_extract_i32
  target/sparc: Use tcg_gen_extract_tl
  target/xtensa: Use tcg_gen_extract_i32
  target/mips: Use tcg_gen_sextract_tl
  target/cris: Use tcg_gen_sextract_tl
  target/ppc: Use tcg_gen_sextract_tl

 target/avr/translate.c          | 18 ++++++------------
 target/cris/translate.c         |  6 ++----
 target/i386/tcg/translate.c     |  9 +++------
 target/mips/tcg/mxu_translate.c |  6 ++----
 target/mips/tcg/translate.c     | 12 ++++--------
 target/ppc/translate.c          | 28 ++++++----------------------
 target/sparc/translate.c        |  6 ++----
 target/xtensa/translate.c       |  6 +-----
 8 files changed, 26 insertions(+), 65 deletions(-)

-- 
2.41.0

[PATCH 1/9] target/avr: Use tcg_gen_extract_tl

[Philippe Mathieu-Daudé]

Inspired-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/avr/translate.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/target/avr/translate.c b/target/avr/translate.c
index cdffa04519..52fa7cebf6 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -223,8 +223,7 @@ static void gen_add_CHf(TCGv R, TCGv Rd, TCGv Rr)
     tcg_gen_or_tl(t1, t1, t3);
 
     tcg_gen_shri_tl(cpu_Cf, t1, 7); /* Cf = t1(7) */
-    tcg_gen_shri_tl(cpu_Hf, t1, 3); /* Hf = t1(3) */
-    tcg_gen_andi_tl(cpu_Hf, cpu_Hf, 1);
+    tcg_gen_extract_tl(cpu_Hf, t1, 3, 1); /* Hf = t1(3) */
 }
 
 static void gen_add_Vf(TCGv R, TCGv Rd, TCGv Rr)
@@ -254,8 +253,7 @@ static void gen_sub_CHf(TCGv R, TCGv Rd, TCGv Rr)
     tcg_gen_or_tl(t2, t2, t3); /* t2 = ~Rd & Rr | ~Rd & R | R & Rr */
 
     tcg_gen_shri_tl(cpu_Cf, t2, 7); /* Cf = t2(7) */
-    tcg_gen_shri_tl(cpu_Hf, t2, 3); /* Hf = t2(3) */
-    tcg_gen_andi_tl(cpu_Hf, cpu_Hf, 1);
+    tcg_gen_extract_tl(cpu_Hf, t2, 3, 1); /* Hf = t2(3) */
 }
 
 static void gen_sub_Vf(TCGv R, TCGv Rd, TCGv Rr)
@@ -810,8 +808,7 @@ static bool trans_FMUL(DisasContext *ctx, arg_FMUL *a)
     /* update output registers */
     tcg_gen_shli_tl(R, R, 1);
     tcg_gen_andi_tl(R0, R, 0xff);
-    tcg_gen_shri_tl(R1, R, 8);
-    tcg_gen_andi_tl(R1, R1, 0xff);
+    tcg_gen_extract_tl(R1, R, 8, 8);
     return true;
 }
 
@@ -845,8 +842,7 @@ static bool trans_FMULS(DisasContext *ctx, arg_FMULS *a)
     /* update output registers */
     tcg_gen_shli_tl(R, R, 1);
     tcg_gen_andi_tl(R0, R, 0xff);
-    tcg_gen_shri_tl(R1, R, 8);
-    tcg_gen_andi_tl(R1, R1, 0xff);
+    tcg_gen_extract_tl(R1, R, 8, 8);
     return true;
 }
 
@@ -878,8 +874,7 @@ static bool trans_FMULSU(DisasContext *ctx, arg_FMULSU *a)
     /* update output registers */
     tcg_gen_shli_tl(R, R, 1);
     tcg_gen_andi_tl(R0, R, 0xff);
-    tcg_gen_shri_tl(R1, R, 8);
-    tcg_gen_andi_tl(R1, R1, 0xff);
+    tcg_gen_extract_tl(R1, R, 8, 8);
     return true;
 }
 
@@ -2020,8 +2015,7 @@ static bool trans_LPMX(DisasContext *ctx, arg_LPMX *a)
     tcg_gen_qemu_ld_tl(Rd, addr, MMU_CODE_IDX, MO_UB);
     tcg_gen_addi_tl(addr, addr, 1); /* addr = addr + 1 */
     tcg_gen_andi_tl(L, addr, 0xff);
-    tcg_gen_shri_tl(addr, addr, 8);
-    tcg_gen_andi_tl(H, addr, 0xff);
+    tcg_gen_extract_tl(H, addr, 8, 8);
     return true;
 }
 
-- 
2.41.0

Re: [PATCH 1/9] target/avr: Use tcg_gen_extract_tl

[Richard Henderson]

On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
> Inspired-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/avr/translate.c | 18 ++++++------------
>   1 file changed, 6 insertions(+), 12 deletions(-)

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

r~

Re: [PATCH 1/9] target/avr: Use tcg_gen_extract_tl

[Michael Rolnik]

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

Reviewed-by: Michael Rolnik <mrolnik@gmail.com>

On Tue, Oct 24, 2023 at 2:32 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
> > Inspired-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   target/avr/translate.c | 18 ++++++------------
> >   1 file changed, 6 insertions(+), 12 deletions(-)
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> r~
>


-- 
Best Regards,
Michael Rolnik

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

[PATCH 2/9] target/cris: Use tcg_gen_extract_tl

[Philippe Mathieu-Daudé]

Inspired-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/cris/translate.c     | 3 +--
 target/i386/tcg/translate.c | 9 +++------
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/target/cris/translate.c b/target/cris/translate.c
index b3974ba0bb..65b07e1d80 100644
--- a/target/cris/translate.c
+++ b/target/cris/translate.c
@@ -871,8 +871,7 @@ static void gen_tst_cc (DisasContext *dc, TCGv cc, int cond)
                 bits = 15;
             }
 
-            tcg_gen_shri_tl(cc, cc_result, bits);
-            tcg_gen_andi_tl(cc, cc, 1);
+            tcg_gen_extract_tl(cc, cc_result, bits, 1);
         } else {
             cris_evaluate_flags(dc);
             tcg_gen_andi_tl(cc, cpu_PR[PR_CCS],
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 587d88692a..25289eeec9 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -1159,8 +1159,7 @@ static void gen_setcc1(DisasContext *s, int b, TCGv reg)
 
     if (cc.cond == TCG_COND_NE && !cc.use_reg2 && cc.imm == 0 &&
         cc.mask != 0 && (cc.mask & (cc.mask - 1)) == 0) {
-        tcg_gen_shri_tl(reg, cc.reg, ctztl(cc.mask));
-        tcg_gen_andi_tl(reg, reg, 1);
+        tcg_gen_extract_tl(reg, cc.reg, ctztl(cc.mask), 1);
         return;
     }
     if (cc.mask != -1) {
@@ -1783,8 +1782,7 @@ static void gen_rot_rm_T1(DisasContext *s, MemOp ot, int op1, int is_right)
        currently dead.  */
     if (is_right) {
         tcg_gen_shri_tl(cpu_cc_src2, s->T0, mask - 1);
-        tcg_gen_shri_tl(cpu_cc_dst, s->T0, mask);
-        tcg_gen_andi_tl(cpu_cc_dst, cpu_cc_dst, 1);
+        tcg_gen_extract_tl(cpu_cc_dst, s->T0, mask, 1);
     } else {
         tcg_gen_shri_tl(cpu_cc_src2, s->T0, mask);
         tcg_gen_andi_tl(cpu_cc_dst, s->T0, 1);
@@ -1873,8 +1871,7 @@ static void gen_rot_rm_im(DisasContext *s, MemOp ot, int op1, int op2,
            currently dead.  */
         if (is_right) {
             tcg_gen_shri_tl(cpu_cc_src2, s->T0, mask - 1);
-            tcg_gen_shri_tl(cpu_cc_dst, s->T0, mask);
-            tcg_gen_andi_tl(cpu_cc_dst, cpu_cc_dst, 1);
+            tcg_gen_extract_tl(cpu_cc_dst, s->T0, mask, 1);
         } else {
             tcg_gen_shri_tl(cpu_cc_src2, s->T0, mask);
             tcg_gen_andi_tl(cpu_cc_dst, s->T0, 1);
-- 
2.41.0

Re: [PATCH 2/9] target/cris: Use tcg_gen_extract_tl

[Richard Henderson]

On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
> Inspired-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/cris/translate.c     | 3 +--
>   target/i386/tcg/translate.c | 9 +++------
>   2 files changed, 4 insertions(+), 8 deletions(-)

Accidental merge of two patches, but they both look fine.  :-)

With the split, to both:
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 2/9] target/cris: Use tcg_gen_extract_tl

[Edgar E. Iglesias]

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

On Tue, Oct 24, 2023 at 1:36 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
> > Inspired-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   target/cris/translate.c     | 3 +--
> >   target/i386/tcg/translate.c | 9 +++------
> >   2 files changed, 4 insertions(+), 8 deletions(-)
>
> Accidental merge of two patches, but they both look fine.  :-)
>
> With the split, to both:
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
>
>
CRIS part looks good with Richard's request to split:
Reviewed-by: Edgar E. Iglesias <edgar@zeroasic.com>



> r~
>

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

[PATCH 3/9] target/mips: Use tcg_gen_extract_i32

[Philippe Mathieu-Daudé]

Inspired-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/mips/tcg/translate.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
index 13e43fa3b6..2586d9c85a 100644
--- a/target/mips/tcg/translate.c
+++ b/target/mips/tcg/translate.c
@@ -1269,8 +1269,7 @@ static inline void gen_load_srsgpr(int from, int to)
         TCGv_ptr addr = tcg_temp_new_ptr();
 
         tcg_gen_ld_i32(t2, tcg_env, offsetof(CPUMIPSState, CP0_SRSCtl));
-        tcg_gen_shri_i32(t2, t2, CP0SRSCtl_PSS);
-        tcg_gen_andi_i32(t2, t2, 0xf);
+        tcg_gen_extract_i32(t2, t2, CP0SRSCtl_PSS, 4);
         tcg_gen_muli_i32(t2, t2, sizeof(target_ulong) * 32);
         tcg_gen_ext_i32_ptr(addr, t2);
         tcg_gen_add_ptr(addr, tcg_env, addr);
@@ -1289,8 +1288,7 @@ static inline void gen_store_srsgpr(int from, int to)
 
         gen_load_gpr(t0, from);
         tcg_gen_ld_i32(t2, tcg_env, offsetof(CPUMIPSState, CP0_SRSCtl));
-        tcg_gen_shri_i32(t2, t2, CP0SRSCtl_PSS);
-        tcg_gen_andi_i32(t2, t2, 0xf);
+        tcg_gen_extract_i32(t2, t2, CP0SRSCtl_PSS, 4);
         tcg_gen_muli_i32(t2, t2, sizeof(target_ulong) * 32);
         tcg_gen_ext_i32_ptr(addr, t2);
         tcg_gen_add_ptr(addr, tcg_env, addr);
@@ -8981,13 +8979,11 @@ static void gen_compute_branch1(DisasContext *ctx, uint32_t op,
         tcg_gen_extu_i32_tl(bcond, t0);
         goto likely;
     case OPC_BC1T:
-        tcg_gen_shri_i32(t0, fpu_fcr31, get_fp_bit(cc));
-        tcg_gen_andi_i32(t0, t0, 1);
+        tcg_gen_extract_i32(t0, fpu_fcr31, get_fp_bit(cc), 1);
         tcg_gen_extu_i32_tl(bcond, t0);
         goto not_likely;
     case OPC_BC1TL:
-        tcg_gen_shri_i32(t0, fpu_fcr31, get_fp_bit(cc));
-        tcg_gen_andi_i32(t0, t0, 1);
+        tcg_gen_extract_i32(t0, fpu_fcr31, get_fp_bit(cc), 1);
         tcg_gen_extu_i32_tl(bcond, t0);
     likely:
         ctx->hflags |= MIPS_HFLAG_BL;
-- 
2.41.0

Re: [PATCH 3/9] target/mips: Use tcg_gen_extract_i32

[Richard Henderson]

On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
> Inspired-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/mips/tcg/translate.c | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)

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


r~

[PATCH 4/9] target/ppc: Use tcg_gen_extract_i32

[Philippe Mathieu-Daudé]

Inspired-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/translate.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 329da4d518..c6e1f7c2ca 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -4802,16 +4802,14 @@ static void gen_mtcrf(DisasContext *ctx)
             TCGv_i32 temp = tcg_temp_new_i32();
             crn = ctz32(crm);
             tcg_gen_trunc_tl_i32(temp, cpu_gpr[rS(ctx->opcode)]);
-            tcg_gen_shri_i32(temp, temp, crn * 4);
-            tcg_gen_andi_i32(cpu_crf[7 - crn], temp, 0xf);
+            tcg_gen_extract_i32(cpu_crf[7 - crn], temp, crn * 4, 4);
         }
     } else {
         TCGv_i32 temp = tcg_temp_new_i32();
         tcg_gen_trunc_tl_i32(temp, cpu_gpr[rS(ctx->opcode)]);
         for (crn = 0 ; crn < 8 ; crn++) {
             if (crm & (1 << crn)) {
-                    tcg_gen_shri_i32(cpu_crf[7 - crn], temp, crn * 4);
-                    tcg_gen_andi_i32(cpu_crf[7 - crn], cpu_crf[7 - crn], 0xf);
+                    tcg_gen_extract_i32(cpu_crf[7 - crn], temp, crn * 4, 4);
             }
         }
     }
-- 
2.41.0

Re: [PATCH 4/9] target/ppc: Use tcg_gen_extract_i32

[Richard Henderson]

On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
> Inspired-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/ppc/translate.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 329da4d518..c6e1f7c2ca 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -4802,16 +4802,14 @@ static void gen_mtcrf(DisasContext *ctx)
>               TCGv_i32 temp = tcg_temp_new_i32();
>               crn = ctz32(crm);
>               tcg_gen_trunc_tl_i32(temp, cpu_gpr[rS(ctx->opcode)]);
> -            tcg_gen_shri_i32(temp, temp, crn * 4);
> -            tcg_gen_andi_i32(cpu_crf[7 - crn], temp, 0xf);
> +            tcg_gen_extract_i32(cpu_crf[7 - crn], temp, crn * 4, 4);
>           }
>       } else {
>           TCGv_i32 temp = tcg_temp_new_i32();
>           tcg_gen_trunc_tl_i32(temp, cpu_gpr[rS(ctx->opcode)]);
>           for (crn = 0 ; crn < 8 ; crn++) {
>               if (crm & (1 << crn)) {
> -                    tcg_gen_shri_i32(cpu_crf[7 - crn], temp, crn * 4);
> -                    tcg_gen_andi_i32(cpu_crf[7 - crn], cpu_crf[7 - crn], 0xf);
> +                    tcg_gen_extract_i32(cpu_crf[7 - crn], temp, crn * 4, 4);

Might as well fix indentation here.

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

>               }
>           }
>       }

[PATCH 5/9] target/sparc: Use tcg_gen_extract_tl

[Philippe Mathieu-Daudé]

Inspired-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/sparc/translate.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index f92ff80ac8..16d9151b90 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -740,14 +740,12 @@ static void gen_op_eval_bvc(TCGv dst, TCGv_i32 src)
 static void gen_mov_reg_FCC0(TCGv reg, TCGv src,
                                     unsigned int fcc_offset)
 {
-    tcg_gen_shri_tl(reg, src, FSR_FCC0_SHIFT + fcc_offset);
-    tcg_gen_andi_tl(reg, reg, 0x1);
+    tcg_gen_extract_tl(reg, src, FSR_FCC0_SHIFT + fcc_offset, 1);
 }
 
 static void gen_mov_reg_FCC1(TCGv reg, TCGv src, unsigned int fcc_offset)
 {
-    tcg_gen_shri_tl(reg, src, FSR_FCC1_SHIFT + fcc_offset);
-    tcg_gen_andi_tl(reg, reg, 0x1);
+    tcg_gen_extract_tl(reg, src, FSR_FCC1_SHIFT + fcc_offset, 1);
 }
 
 // !0: FCC0 | FCC1
-- 
2.41.0

Re: [PATCH 5/9] target/sparc: Use tcg_gen_extract_tl

[Richard Henderson]

On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
> Inspired-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/sparc/translate.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/target/sparc/translate.c b/target/sparc/translate.c
> index f92ff80ac8..16d9151b90 100644
> --- a/target/sparc/translate.c
> +++ b/target/sparc/translate.c
> @@ -740,14 +740,12 @@ static void gen_op_eval_bvc(TCGv dst, TCGv_i32 src)
>   static void gen_mov_reg_FCC0(TCGv reg, TCGv src,
>                                       unsigned int fcc_offset)
>   {
> -    tcg_gen_shri_tl(reg, src, FSR_FCC0_SHIFT + fcc_offset);
> -    tcg_gen_andi_tl(reg, reg, 0x1);
> +    tcg_gen_extract_tl(reg, src, FSR_FCC0_SHIFT + fcc_offset, 1);
>   }
>   
>   static void gen_mov_reg_FCC1(TCGv reg, TCGv src, unsigned int fcc_offset)
>   {
> -    tcg_gen_shri_tl(reg, src, FSR_FCC1_SHIFT + fcc_offset);
> -    tcg_gen_andi_tl(reg, reg, 0x1);
> +    tcg_gen_extract_tl(reg, src, FSR_FCC1_SHIFT + fcc_offset, 1);
>   }

I have patches as yet not published which remove these entirely.

But this is correct in the meantime,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 6/9] target/xtensa: Use tcg_gen_extract_i32

[Philippe Mathieu-Daudé]

Inspired-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/xtensa/translate.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index de89940599..cbc564c020 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -1595,11 +1595,7 @@ static void translate_entry(DisasContext *dc, const OpcodeArg arg[],
 static void translate_extui(DisasContext *dc, const OpcodeArg arg[],
                             const uint32_t par[])
 {
-    int maskimm = (1 << arg[3].imm) - 1;
-
-    TCGv_i32 tmp = tcg_temp_new_i32();
-    tcg_gen_shri_i32(tmp, arg[1].in, arg[2].imm);
-    tcg_gen_andi_i32(arg[0].out, tmp, maskimm);
+    tcg_gen_extract_i32(arg[0].out, arg[1].in, arg[2].imm, arg[3].imm);
 }
 
 static void translate_getex(DisasContext *dc, const OpcodeArg arg[],
-- 
2.41.0

Re: [PATCH 6/9] target/xtensa: Use tcg_gen_extract_i32

[Max Filippov]

On Mon, Oct 23, 2023 at 9:10 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Inspired-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  target/xtensa/translate.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)

Reviewed-by: Max Filippov <jcmvbkbc@gmail.com>

-- 
Thanks.
-- Max

[PATCH 7/9] target/mips: Use tcg_gen_sextract_tl

[Philippe Mathieu-Daudé]

Inspired-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/mips/tcg/mxu_translate.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/target/mips/tcg/mxu_translate.c b/target/mips/tcg/mxu_translate.c
index c517258ac5..6eb73256b2 100644
--- a/target/mips/tcg/mxu_translate.c
+++ b/target/mips/tcg/mxu_translate.c
@@ -3840,8 +3840,7 @@ static void gen_mxu_Q16SAT(DisasContext *ctx)
             tcg_gen_movi_tl(t0, 255);
 
             gen_set_label(l_lo);
-            tcg_gen_shli_tl(t1, mxu_gpr[XRb - 1], 16);
-            tcg_gen_sari_tl(t1, t1, 16);
+            tcg_gen_sextract_tl(t1, mxu_gpr[XRb - 1], 0, 16);
             tcg_gen_brcondi_tl(TCG_COND_LT, t1, 0, l_less_lo);
             tcg_gen_brcondi_tl(TCG_COND_GT, t1, 255, l_greater_lo);
             tcg_gen_br(l_done);
@@ -3876,8 +3875,7 @@ static void gen_mxu_Q16SAT(DisasContext *ctx)
             tcg_gen_movi_tl(t0, 255);
 
             gen_set_label(l_lo);
-            tcg_gen_shli_tl(t1, mxu_gpr[XRc - 1], 16);
-            tcg_gen_sari_tl(t1, t1, 16);
+            tcg_gen_sextract_tl(t1, mxu_gpr[XRc - 1], 0, 16);
             tcg_gen_brcondi_tl(TCG_COND_LT, t1, 0, l_less_lo);
             tcg_gen_brcondi_tl(TCG_COND_GT, t1, 255, l_greater_lo);
             tcg_gen_br(l_done);
-- 
2.41.0

Re: [PATCH 7/9] target/mips: Use tcg_gen_sextract_tl

[Richard Henderson]

On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
> Inspired-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/mips/tcg/mxu_translate.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/target/mips/tcg/mxu_translate.c b/target/mips/tcg/mxu_translate.c
> index c517258ac5..6eb73256b2 100644
> --- a/target/mips/tcg/mxu_translate.c
> +++ b/target/mips/tcg/mxu_translate.c
> @@ -3840,8 +3840,7 @@ static void gen_mxu_Q16SAT(DisasContext *ctx)
>               tcg_gen_movi_tl(t0, 255);
>   
>               gen_set_label(l_lo);
> -            tcg_gen_shli_tl(t1, mxu_gpr[XRb - 1], 16);
> -            tcg_gen_sari_tl(t1, t1, 16);
> +            tcg_gen_sextract_tl(t1, mxu_gpr[XRb - 1], 0, 16);

The most simple replacement here is tcg_gen_ext16s_tl.

I'll also note that the entire function should be replaced, e.g.

     TCGv min = tcg_constant_tl(0);
     TCGv max = tcg_constant_tl(0xff);
     TCGv tmp[2];

     tmp[0] = tcg_temp_new();
     tmp[1] = tcg_temp_new();

     for (i = 0; i < 4; i++) {
         int rs = i & 2 ? XRc : XRb;
         TCGv t = tmp[i & 1];

         gen_load_mxu_gpr(t, rs);
         tcg_gen_sextract_tl(t, t, (i & 1) * 16, 16);
         tcg_gen_smax_tl(t, t, min);
         tcg_gen_smin_tl(t, t, max);
         if (i != 0) {
             tcg_gen_shli_tl(t, t, i * 8);
             tcg_gen_or_tl(t, t, tmp[(i - 1) & 1];
         }
     }
     gen_store_mxu_gpr(tmp[1], XRa);


And lots of other similar work within this file.  :-/


r~

Re: [PATCH 7/9] target/mips: Use tcg_gen_sextract_tl

[Philippe Mathieu-Daudé]

On 24/10/23 02:14, Richard Henderson wrote:
> On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
>> Inspired-by: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/mips/tcg/mxu_translate.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/mips/tcg/mxu_translate.c 
>> b/target/mips/tcg/mxu_translate.c
>> index c517258ac5..6eb73256b2 100644
>> --- a/target/mips/tcg/mxu_translate.c
>> +++ b/target/mips/tcg/mxu_translate.c
>> @@ -3840,8 +3840,7 @@ static void gen_mxu_Q16SAT(DisasContext *ctx)
>>               tcg_gen_movi_tl(t0, 255);
>>               gen_set_label(l_lo);
>> -            tcg_gen_shli_tl(t1, mxu_gpr[XRb - 1], 16);
>> -            tcg_gen_sari_tl(t1, t1, 16);
>> +            tcg_gen_sextract_tl(t1, mxu_gpr[XRb - 1], 0, 16);
> 
> The most simple replacement here is tcg_gen_ext16s_tl.
> 
> I'll also note that the entire function should be replaced, e.g.
> 
>      TCGv min = tcg_constant_tl(0);
>      TCGv max = tcg_constant_tl(0xff);
>      TCGv tmp[2];
> 
>      tmp[0] = tcg_temp_new();
>      tmp[1] = tcg_temp_new();
> 
>      for (i = 0; i < 4; i++) {
>          int rs = i & 2 ? XRc : XRb;
>          TCGv t = tmp[i & 1];
> 
>          gen_load_mxu_gpr(t, rs);
>          tcg_gen_sextract_tl(t, t, (i & 1) * 16, 16);
>          tcg_gen_smax_tl(t, t, min);
>          tcg_gen_smin_tl(t, t, max);
>          if (i != 0) {
>              tcg_gen_shli_tl(t, t, i * 8);
>              tcg_gen_or_tl(t, t, tmp[(i - 1) & 1];
>          }
>      }
>      gen_store_mxu_gpr(tmp[1], XRa);
> 
> 
> And lots of other similar work within this file.  :-/

Yeah, this code is upported from some old tree. It should
use the TCG GVec API. See:
https://lore.kernel.org/qemu-devel/781894e9-2565-b54f-8df3-9cbd1cf68e2a@linaro.org/

Re: [PATCH 7/9] target/mips: Use tcg_gen_sextract_tl

[Philippe Mathieu-Daudé]

On 24/10/23 10:57, Philippe Mathieu-Daudé wrote:
> On 24/10/23 02:14, Richard Henderson wrote:
>> On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
>>> Inspired-by: Richard Henderson <richard.henderson@linaro.org>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   target/mips/tcg/mxu_translate.c | 6 ++----
>>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/target/mips/tcg/mxu_translate.c 
>>> b/target/mips/tcg/mxu_translate.c
>>> index c517258ac5..6eb73256b2 100644
>>> --- a/target/mips/tcg/mxu_translate.c
>>> +++ b/target/mips/tcg/mxu_translate.c
>>> @@ -3840,8 +3840,7 @@ static void gen_mxu_Q16SAT(DisasContext *ctx)
>>>               tcg_gen_movi_tl(t0, 255);
>>>               gen_set_label(l_lo);
>>> -            tcg_gen_shli_tl(t1, mxu_gpr[XRb - 1], 16);
>>> -            tcg_gen_sari_tl(t1, t1, 16);
>>> +            tcg_gen_sextract_tl(t1, mxu_gpr[XRb - 1], 0, 16);
>>
>> The most simple replacement here is tcg_gen_ext16s_tl.
>>
>> I'll also note that the entire function should be replaced, e.g.
>>
>>      TCGv min = tcg_constant_tl(0);
>>      TCGv max = tcg_constant_tl(0xff);
>>      TCGv tmp[2];
>>
>>      tmp[0] = tcg_temp_new();
>>      tmp[1] = tcg_temp_new();
>>
>>      for (i = 0; i < 4; i++) {
>>          int rs = i & 2 ? XRc : XRb;
>>          TCGv t = tmp[i & 1];
>>
>>          gen_load_mxu_gpr(t, rs);
>>          tcg_gen_sextract_tl(t, t, (i & 1) * 16, 16);
>>          tcg_gen_smax_tl(t, t, min);
>>          tcg_gen_smin_tl(t, t, max);
>>          if (i != 0) {
>>              tcg_gen_shli_tl(t, t, i * 8);
>>              tcg_gen_or_tl(t, t, tmp[(i - 1) & 1];
>>          }
>>      }
>>      gen_store_mxu_gpr(tmp[1], XRa);

I'll tag your suggestion for later, thanks!

>> And lots of other similar work within this file.  :-/
> 
> Yeah, this code is upported from some old tree. It should
> use the TCG GVec API. See:
> https://lore.kernel.org/qemu-devel/781894e9-2565-b54f-8df3-9cbd1cf68e2a@linaro.org/
> 

[RFC PATCH 8/9] target/cris: Use tcg_gen_sextract_tl

[Philippe Mathieu-Daudé]

Inspired-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC: please double-check bits
---
 target/cris/translate.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/cris/translate.c b/target/cris/translate.c
index 65b07e1d80..3a161f8f73 100644
--- a/target/cris/translate.c
+++ b/target/cris/translate.c
@@ -336,8 +336,7 @@ static void t_gen_cris_mstep(TCGv d, TCGv a, TCGv b, TCGv ccs)
      */
     t = tcg_temp_new();
     tcg_gen_shli_tl(d, a, 1);
-    tcg_gen_shli_tl(t, ccs, 31 - 3);
-    tcg_gen_sari_tl(t, t, 31);
+    tcg_gen_sextract_tl(t, ccs, 3, 1);
     tcg_gen_and_tl(t, t, b);
     tcg_gen_add_tl(d, d, t);
 }
-- 
2.41.0

Re: [RFC PATCH 8/9] target/cris: Use tcg_gen_sextract_tl

[Richard Henderson]

On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
> Inspired-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> RFC: please double-check bits
> ---
>   target/cris/translate.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/target/cris/translate.c b/target/cris/translate.c
> index 65b07e1d80..3a161f8f73 100644
> --- a/target/cris/translate.c
> +++ b/target/cris/translate.c
> @@ -336,8 +336,7 @@ static void t_gen_cris_mstep(TCGv d, TCGv a, TCGv b, TCGv ccs)
>        */
>       t = tcg_temp_new();
>       tcg_gen_shli_tl(d, a, 1);
> -    tcg_gen_shli_tl(t, ccs, 31 - 3);
> -    tcg_gen_sari_tl(t, t, 31);
> +    tcg_gen_sextract_tl(t, ccs, 3, 1);

tcg_gen_sextract_tl(t, ccs, ctz32(N_FLAG), 1);

Also, it appears t_gen_cris_mstep consumes CCS without making sure that it is up-to-date.
Edgar?


r~

Re: [RFC PATCH 8/9] target/cris: Use tcg_gen_sextract_tl

[Edgar E. Iglesias]

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

On Tue, Oct 24, 2023 at 2:26 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
> > Inspired-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> > RFC: please double-check bits
> > ---
> >   target/cris/translate.c | 3 +--
> >   1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/target/cris/translate.c b/target/cris/translate.c
> > index 65b07e1d80..3a161f8f73 100644
> > --- a/target/cris/translate.c
> > +++ b/target/cris/translate.c
> > @@ -336,8 +336,7 @@ static void t_gen_cris_mstep(TCGv d, TCGv a, TCGv b,
> TCGv ccs)
> >        */
> >       t = tcg_temp_new();
> >       tcg_gen_shli_tl(d, a, 1);
> > -    tcg_gen_shli_tl(t, ccs, 31 - 3);
> > -    tcg_gen_sari_tl(t, t, 31);
> > +    tcg_gen_sextract_tl(t, ccs, 3, 1);


> tcg_gen_sextract_tl(t, ccs, ctz32(N_FLAG), 1);
>

Looks good!

I think the intent was a branch-free version of:
if (ccs & N_FLAG) {
    d += b;
}

Or:
t = ccs & N_FLAG ? UINT32_MAX : 0;
d += b & t;



>
> Also, it appears t_gen_cris_mstep consumes CCS without making sure that it
> is up-to-date.
> Edgar?
>
>
Yes, that looks suspicious!

Best regards,
Edgar


>
> r~
>

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

Re: [RFC PATCH 8/9] target/cris: Use tcg_gen_sextract_tl

[Philippe Mathieu-Daudé]

On 24/10/23 02:26, Richard Henderson wrote:
> On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
>> Inspired-by: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> RFC: please double-check bits
>> ---
>>   target/cris/translate.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/target/cris/translate.c b/target/cris/translate.c
>> index 65b07e1d80..3a161f8f73 100644
>> --- a/target/cris/translate.c
>> +++ b/target/cris/translate.c
>> @@ -336,8 +336,7 @@ static void t_gen_cris_mstep(TCGv d, TCGv a, TCGv 
>> b, TCGv ccs)
>>        */
>>       t = tcg_temp_new();
>>       tcg_gen_shli_tl(d, a, 1);
>> -    tcg_gen_shli_tl(t, ccs, 31 - 3);
>> -    tcg_gen_sari_tl(t, t, 31);
>> +    tcg_gen_sextract_tl(t, ccs, 3, 1);
> 
> tcg_gen_sextract_tl(t, ccs, ctz32(N_FLAG), 1);
> 
> Also, it appears t_gen_cris_mstep consumes CCS without making sure that 
> it is up-to-date.

Do you mean we first need to call cris_evaluate_flags?

-- >8 --
diff --git a/target/cris/translate.c b/target/cris/translate.c
index 3a161f8f73..5eb68b8a63 100644
--- a/target/cris/translate.c
+++ b/target/cris/translate.c
@@ -177,6 +177,8 @@ static const int preg_sizes[] = {
  #define t_gen_movi_env_TN(member, c) \
      t_gen_mov_env_TN(member, tcg_constant_tl(c))

+static void cris_evaluate_flags(DisasContext *dc);
+
  static inline void t_gen_mov_TN_preg(TCGv tn, int r)
  {
      assert(r >= 0 && r <= 15);
@@ -325,7 +327,7 @@ static void t_gen_cris_dstep(TCGv d, TCGv a, TCGv b)
      tcg_gen_movcond_tl(TCG_COND_GEU, d, d, b, t, d);
  }

-static void t_gen_cris_mstep(TCGv d, TCGv a, TCGv b, TCGv ccs)
+static void t_gen_cris_mstep(DisasContext *dc, TCGv d, TCGv a, TCGv b, 
TCGv ccs)
  {
      TCGv t;

@@ -335,6 +337,7 @@ static void t_gen_cris_mstep(TCGv d, TCGv a, TCGv b, 
TCGv ccs)
       *    d += s;
       */
      t = tcg_temp_new();
+    cris_evaluate_flags(dc);
      tcg_gen_shli_tl(d, a, 1);
      tcg_gen_sextract_tl(t, ccs, 3, 1);
      tcg_gen_and_tl(t, t, b);
@@ -702,7 +705,7 @@ static void cris_alu_op_exec(DisasContext *dc, int op,
          t_gen_cris_dstep(dst, a, b);
          break;
      case CC_OP_MSTEP:
-        t_gen_cris_mstep(dst, a, b, cpu_PR[PR_CCS]);
+        t_gen_cris_mstep(dc, dst, a, b, cpu_PR[PR_CCS]);
          break;
      case CC_OP_BOUND:
          tcg_gen_movcond_tl(TCG_COND_LEU, dst, a, b, a, b);
---

Re: [RFC PATCH 8/9] target/cris: Use tcg_gen_sextract_tl

[Philippe Mathieu-Daudé]

On 24/10/23 10:53, Philippe Mathieu-Daudé wrote:
> On 24/10/23 02:26, Richard Henderson wrote:
>> On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
>>> Inspired-by: Richard Henderson <richard.henderson@linaro.org>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> RFC: please double-check bits
>>> ---
>>>   target/cris/translate.c | 3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/target/cris/translate.c b/target/cris/translate.c
>>> index 65b07e1d80..3a161f8f73 100644
>>> --- a/target/cris/translate.c
>>> +++ b/target/cris/translate.c
>>> @@ -336,8 +336,7 @@ static void t_gen_cris_mstep(TCGv d, TCGv a, TCGv 
>>> b, TCGv ccs)
>>>        */
>>>       t = tcg_temp_new();
>>>       tcg_gen_shli_tl(d, a, 1);
>>> -    tcg_gen_shli_tl(t, ccs, 31 - 3);
>>> -    tcg_gen_sari_tl(t, t, 31);
>>> +    tcg_gen_sextract_tl(t, ccs, 3, 1);
>>
>> tcg_gen_sextract_tl(t, ccs, ctz32(N_FLAG), 1);
>>
>> Also, it appears t_gen_cris_mstep consumes CCS without making sure 
>> that it is up-to-date.
> 
> Do you mean we first need to call cris_evaluate_flags?
> 
> -- >8 --
> diff --git a/target/cris/translate.c b/target/cris/translate.c
> index 3a161f8f73..5eb68b8a63 100644
> --- a/target/cris/translate.c
> +++ b/target/cris/translate.c
> @@ -177,6 +177,8 @@ static const int preg_sizes[] = {
>   #define t_gen_movi_env_TN(member, c) \
>       t_gen_mov_env_TN(member, tcg_constant_tl(c))
> 
> +static void cris_evaluate_flags(DisasContext *dc);
> +
>   static inline void t_gen_mov_TN_preg(TCGv tn, int r)
>   {
>       assert(r >= 0 && r <= 15);
> @@ -325,7 +327,7 @@ static void t_gen_cris_dstep(TCGv d, TCGv a, TCGv b)
>       tcg_gen_movcond_tl(TCG_COND_GEU, d, d, b, t, d);
>   }
> 
> -static void t_gen_cris_mstep(TCGv d, TCGv a, TCGv b, TCGv ccs)
> +static void t_gen_cris_mstep(DisasContext *dc, TCGv d, TCGv a, TCGv b, 
> TCGv ccs)
>   {
>       TCGv t;
> 
> @@ -335,6 +337,7 @@ static void t_gen_cris_mstep(TCGv d, TCGv a, TCGv b, 
> TCGv ccs)
>        *    d += s;
>        */
>       t = tcg_temp_new();
> +    cris_evaluate_flags(dc);
>       tcg_gen_shli_tl(d, a, 1);
>       tcg_gen_sextract_tl(t, ccs, 3, 1);

Err, to be applied before this patch #8.

>       tcg_gen_and_tl(t, t, b);
> @@ -702,7 +705,7 @@ static void cris_alu_op_exec(DisasContext *dc, int op,
>           t_gen_cris_dstep(dst, a, b);
>           break;
>       case CC_OP_MSTEP:
> -        t_gen_cris_mstep(dst, a, b, cpu_PR[PR_CCS]);
> +        t_gen_cris_mstep(dc, dst, a, b, cpu_PR[PR_CCS]);
>           break;
>       case CC_OP_BOUND:
>           tcg_gen_movcond_tl(TCG_COND_LEU, dst, a, b, a, b);
> ---
> 

[RFC PATCH 9/9] target/ppc: Use tcg_gen_sextract_tl

[Philippe Mathieu-Daudé]

Inspired-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC: Please double-check 32/64 & bits
---
 target/ppc/translate.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index c6e1f7c2ca..1370db9bd5 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -2892,13 +2892,7 @@ static void gen_slw(DisasContext *ctx)
 
     t0 = tcg_temp_new();
     /* AND rS with a mask that is 0 when rB >= 0x20 */
-#if defined(TARGET_PPC64)
-    tcg_gen_shli_tl(t0, cpu_gpr[rB(ctx->opcode)], 0x3a);
-    tcg_gen_sari_tl(t0, t0, 0x3f);
-#else
-    tcg_gen_shli_tl(t0, cpu_gpr[rB(ctx->opcode)], 0x1a);
-    tcg_gen_sari_tl(t0, t0, 0x1f);
-#endif
+    tcg_gen_sextract_tl(t0, cpu_gpr[rB(ctx->opcode)], 5, 1);
     tcg_gen_andc_tl(t0, cpu_gpr[rS(ctx->opcode)], t0);
     t1 = tcg_temp_new();
     tcg_gen_andi_tl(t1, cpu_gpr[rB(ctx->opcode)], 0x1f);
@@ -2956,13 +2950,7 @@ static void gen_srw(DisasContext *ctx)
 
     t0 = tcg_temp_new();
     /* AND rS with a mask that is 0 when rB >= 0x20 */
-#if defined(TARGET_PPC64)
-    tcg_gen_shli_tl(t0, cpu_gpr[rB(ctx->opcode)], 0x3a);
-    tcg_gen_sari_tl(t0, t0, 0x3f);
-#else
-    tcg_gen_shli_tl(t0, cpu_gpr[rB(ctx->opcode)], 0x1a);
-    tcg_gen_sari_tl(t0, t0, 0x1f);
-#endif
+    tcg_gen_sextract_tl(t0, cpu_gpr[rB(ctx->opcode)], 5, 1);
     tcg_gen_andc_tl(t0, cpu_gpr[rS(ctx->opcode)], t0);
     tcg_gen_ext32u_tl(t0, t0);
     t1 = tcg_temp_new();
@@ -2981,8 +2969,7 @@ static void gen_sld(DisasContext *ctx)
 
     t0 = tcg_temp_new();
     /* AND rS with a mask that is 0 when rB >= 0x40 */
-    tcg_gen_shli_tl(t0, cpu_gpr[rB(ctx->opcode)], 0x39);
-    tcg_gen_sari_tl(t0, t0, 0x3f);
+    tcg_gen_sextract_tl(t0, cpu_gpr[rB(ctx->opcode)], 6, 1);
     tcg_gen_andc_tl(t0, cpu_gpr[rS(ctx->opcode)], t0);
     t1 = tcg_temp_new();
     tcg_gen_andi_tl(t1, cpu_gpr[rB(ctx->opcode)], 0x3f);
@@ -3071,8 +3058,7 @@ static void gen_srd(DisasContext *ctx)
 
     t0 = tcg_temp_new();
     /* AND rS with a mask that is 0 when rB >= 0x40 */
-    tcg_gen_shli_tl(t0, cpu_gpr[rB(ctx->opcode)], 0x39);
-    tcg_gen_sari_tl(t0, t0, 0x3f);
+    tcg_gen_sextract_tl(t0, cpu_gpr[rB(ctx->opcode)], 6, 1);
     tcg_gen_andc_tl(t0, cpu_gpr[rS(ctx->opcode)], t0);
     t1 = tcg_temp_new();
     tcg_gen_andi_tl(t1, cpu_gpr[rB(ctx->opcode)], 0x3f);
-- 
2.41.0

Re: [RFC PATCH 9/9] target/ppc: Use tcg_gen_sextract_tl

[Richard Henderson]

On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
> Inspired-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> RFC: Please double-check 32/64 & bits
> ---
>   target/ppc/translate.c | 22 ++++------------------
>   1 file changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index c6e1f7c2ca..1370db9bd5 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -2892,13 +2892,7 @@ static void gen_slw(DisasContext *ctx)
>   
>       t0 = tcg_temp_new();
>       /* AND rS with a mask that is 0 when rB >= 0x20 */
> -#if defined(TARGET_PPC64)
> -    tcg_gen_shli_tl(t0, cpu_gpr[rB(ctx->opcode)], 0x3a);
> -    tcg_gen_sari_tl(t0, t0, 0x3f);
> -#else
> -    tcg_gen_shli_tl(t0, cpu_gpr[rB(ctx->opcode)], 0x1a);
> -    tcg_gen_sari_tl(t0, t0, 0x1f);
> -#endif
> +    tcg_gen_sextract_tl(t0, cpu_gpr[rB(ctx->opcode)], 5, 1);
>       tcg_gen_andc_tl(t0, cpu_gpr[rS(ctx->opcode)], t0);

Patch looks correct as is, so
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


However:
I'd be tempted to use and+movcond instead of sext+andc.
Also there is a special case of 32-bit shifts with 64-bit shift count on ppc64.

#ifdef TARGET_PPC64
     tcg_gen_andi_tl(t0, rb, 0x3f);
#else
     tcg_gen_andi_tl(t0, rb, 0x1f);
     tcg_gen_andi_tl(t1, rb, 0x20);
     tcg_gen_movcond_tl(TCG_COND_NE, t1, t1, zero, zero, rs);
     rs = t1;
#endif
     tcg_gen_shl_tl(ra, rs, t0);
     tcg_gen_ext32u_tl(ra, ra);


It also makes me wonder about adding some TCGCond for bit-test so that this could be

     tcg_gen_movcond_tl(TCG_COND_TSTNE, t1, rb, 0x20, 0, 0, rs);

and make use of the "test" vs "cmp" instructions on most hosts, but especially x86.


r~

Re: [RFC PATCH 9/9] target/ppc: Use tcg_gen_sextract_tl

[Nicholas Piggin]

On Tue Oct 24, 2023 at 11:04 AM AEST, Richard Henderson wrote:
> On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
> > Inspired-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> > RFC: Please double-check 32/64 & bits
> > ---
> >   target/ppc/translate.c | 22 ++++------------------
> >   1 file changed, 4 insertions(+), 18 deletions(-)
> > 
> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index c6e1f7c2ca..1370db9bd5 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -2892,13 +2892,7 @@ static void gen_slw(DisasContext *ctx)
> >   
> >       t0 = tcg_temp_new();
> >       /* AND rS with a mask that is 0 when rB >= 0x20 */
> > -#if defined(TARGET_PPC64)
> > -    tcg_gen_shli_tl(t0, cpu_gpr[rB(ctx->opcode)], 0x3a);
> > -    tcg_gen_sari_tl(t0, t0, 0x3f);
> > -#else
> > -    tcg_gen_shli_tl(t0, cpu_gpr[rB(ctx->opcode)], 0x1a);
> > -    tcg_gen_sari_tl(t0, t0, 0x1f);
> > -#endif
> > +    tcg_gen_sextract_tl(t0, cpu_gpr[rB(ctx->opcode)], 5, 1);
> >       tcg_gen_andc_tl(t0, cpu_gpr[rS(ctx->opcode)], t0);
>
> Patch looks correct as is, so
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>
>
> However:
> I'd be tempted to use and+movcond instead of sext+andc.

That would be simpler / more mechnical following of specification
in the ISA. Might be better to save that for a later patch though.
Any downsides for backend generation? On host without cmov?

> Also there is a special case of 32-bit shifts with 64-bit shift count on ppc64.
>
> #ifdef TARGET_PPC64
>      tcg_gen_andi_tl(t0, rb, 0x3f);
> #else
>      tcg_gen_andi_tl(t0, rb, 0x1f);
>      tcg_gen_andi_tl(t1, rb, 0x20);
>      tcg_gen_movcond_tl(TCG_COND_NE, t1, t1, zero, zero, rs);
>      rs = t1;
> #endif
>      tcg_gen_shl_tl(ra, rs, t0);
>      tcg_gen_ext32u_tl(ra, ra);
>
>
> It also makes me wonder about adding some TCGCond for bit-test so that this could be
>
>      tcg_gen_movcond_tl(TCG_COND_TSTNE, t1, rb, 0x20, 0, 0, rs);
>
> and make use of the "test" vs "cmp" instructions on most hosts, but especially x86.

Might be useful.

Thanks,
Nick

Re: [RFC PATCH 9/9] target/ppc: Use tcg_gen_sextract_tl

[Philippe Mathieu-Daudé]

Hi Nicholas,

On 25/10/23 09:09, Nicholas Piggin wrote:
> On Tue Oct 24, 2023 at 11:04 AM AEST, Richard Henderson wrote:
>> On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
>>> Inspired-by: Richard Henderson <richard.henderson@linaro.org>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> RFC: Please double-check 32/64 & bits
>>> ---
>>>    target/ppc/translate.c | 22 ++++------------------
>>>    1 file changed, 4 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>>> index c6e1f7c2ca..1370db9bd5 100644
>>> --- a/target/ppc/translate.c
>>> +++ b/target/ppc/translate.c
>>> @@ -2892,13 +2892,7 @@ static void gen_slw(DisasContext *ctx)
>>>    
>>>        t0 = tcg_temp_new();
>>>        /* AND rS with a mask that is 0 when rB >= 0x20 */
>>> -#if defined(TARGET_PPC64)
>>> -    tcg_gen_shli_tl(t0, cpu_gpr[rB(ctx->opcode)], 0x3a);
>>> -    tcg_gen_sari_tl(t0, t0, 0x3f);
>>> -#else
>>> -    tcg_gen_shli_tl(t0, cpu_gpr[rB(ctx->opcode)], 0x1a);
>>> -    tcg_gen_sari_tl(t0, t0, 0x1f);
>>> -#endif
>>> +    tcg_gen_sextract_tl(t0, cpu_gpr[rB(ctx->opcode)], 5, 1);
>>>        tcg_gen_andc_tl(t0, cpu_gpr[rS(ctx->opcode)], t0);
>>
>> Patch looks correct as is, so
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

So are you OK to take this patch as-is as a first iteration?

>> However:
>> I'd be tempted to use and+movcond instead of sext+andc.
> 
> That would be simpler / more mechnical following of specification
> in the ISA. Might be better to save that for a later patch though.
> Any downsides for backend generation? On host without cmov?
> 
>> Also there is a special case of 32-bit shifts with 64-bit shift count on ppc64.
>>
>> #ifdef TARGET_PPC64
>>       tcg_gen_andi_tl(t0, rb, 0x3f);
>> #else
>>       tcg_gen_andi_tl(t0, rb, 0x1f);
>>       tcg_gen_andi_tl(t1, rb, 0x20);
>>       tcg_gen_movcond_tl(TCG_COND_NE, t1, t1, zero, zero, rs);
>>       rs = t1;
>> #endif
>>       tcg_gen_shl_tl(ra, rs, t0);
>>       tcg_gen_ext32u_tl(ra, ra);
>>
>>
>> It also makes me wonder about adding some TCGCond for bit-test so that this could be
>>
>>       tcg_gen_movcond_tl(TCG_COND_TSTNE, t1, rb, 0x20, 0, 0, rs);
>>
>> and make use of the "test" vs "cmp" instructions on most hosts, but especially x86.
> 
> Might be useful.

Now closer:
https://lore.kernel.org/qemu-devel/20231025072707.833943-1-richard.henderson@linaro.org/

:)

Re: [RFC PATCH 9/9] target/ppc: Use tcg_gen_sextract_tl

[Richard Henderson]

On 10/25/23 00:33, Philippe Mathieu-Daudé wrote:
> Hi Nicholas,
> 
> On 25/10/23 09:09, Nicholas Piggin wrote:
>> On Tue Oct 24, 2023 at 11:04 AM AEST, Richard Henderson wrote:
>>> On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
>>>> Inspired-by: Richard Henderson <richard.henderson@linaro.org>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>> RFC: Please double-check 32/64 & bits
>>>> ---
>>>>    target/ppc/translate.c | 22 ++++------------------
>>>>    1 file changed, 4 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>>>> index c6e1f7c2ca..1370db9bd5 100644
>>>> --- a/target/ppc/translate.c
>>>> +++ b/target/ppc/translate.c
>>>> @@ -2892,13 +2892,7 @@ static void gen_slw(DisasContext *ctx)
>>>>        t0 = tcg_temp_new();
>>>>        /* AND rS with a mask that is 0 when rB >= 0x20 */
>>>> -#if defined(TARGET_PPC64)
>>>> -    tcg_gen_shli_tl(t0, cpu_gpr[rB(ctx->opcode)], 0x3a);
>>>> -    tcg_gen_sari_tl(t0, t0, 0x3f);
>>>> -#else
>>>> -    tcg_gen_shli_tl(t0, cpu_gpr[rB(ctx->opcode)], 0x1a);
>>>> -    tcg_gen_sari_tl(t0, t0, 0x1f);
>>>> -#endif
>>>> +    tcg_gen_sextract_tl(t0, cpu_gpr[rB(ctx->opcode)], 5, 1);
>>>>        tcg_gen_andc_tl(t0, cpu_gpr[rS(ctx->opcode)], t0);
>>>
>>> Patch looks correct as is, so
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>
>> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> 
> So are you OK to take this patch as-is as a first iteration?

I am, yes.  The simple fact of the ifdef removal is worth it.


r~

Re: [RFC PATCH 9/9] target/ppc: Use tcg_gen_sextract_tl

[Richard Henderson]

On 10/25/23 00:09, Nicholas Piggin wrote:
>> I'd be tempted to use and+movcond instead of sext+andc.
> 
> That would be simpler / more mechnical following of specification
> in the ISA. Might be better to save that for a later patch though.
> Any downsides for backend generation? On host without cmov?

Probably not enough to worry about.

Virtually all extant hosts do have cmov.  Those that don't have it as part of the ISA *do* 
have it in the TCG backend, implemented as branch over next, for minimal code size.


r~