| Message ID | 20231023160944.10692-9-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | tcg: Use tcg_gen_[s]extract_{i32,i64,tl} | expand |
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~
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~ >
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); ---
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); > --- >
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); }
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(-)