Re: [PATCH v2 2/5] target/ppc/power8-pmu-insn-cnt: introduce inc_spr_if_cond()

[Richard Henderson]

On 12/23/21 12:18 PM, Daniel Henrique Barboza wrote:
> The code that increments a PMC is repetitive: check if a given register
> has a bit/mask set or cleared and increment the counter.
>
> inc_spr_if_cond() will help deal with this repetition. This patch also
> gives a sample of how the function works by incrementing PMC5, which is
> supposed to be incremented only if MMCR0_FC56 is not set.
>
> We've also removing the call from the helper since that would cause
> PMC5 to be counted twice.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   target/ppc/power8-pmu-insn-cnt.c.inc | 43 ++++++++++++++++++++++------
>   1 file changed, 34 insertions(+), 9 deletions(-)
>
> diff --git a/target/ppc/power8-pmu-insn-cnt.c.inc 
> b/target/ppc/power8-pmu-insn-cnt.c.inc
> index 6cdf2d2d88..3cfb801c69 100644
> --- a/target/ppc/power8-pmu-insn-cnt.c.inc
> +++ b/target/ppc/power8-pmu-insn-cnt.c.inc
> @@ -10,6 +10,38 @@
>    * See the COPYING file in the top-level directory.
>    */
>   
> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> +/*
> + * This function increments a SPR 'spr' by 'inc_val' if a given
> + * register 'reg' has a bitmask 'mask' set (cond = TCG_COND_EQ) or
> + * not set (TCG_COND_NE).
> + */
> +static void inc_spr_if_cond(int reg, uint64_t mask, TCGCond cond,
> +                            int spr, int inc_val)
> +{
> +    TCGCond exit_cond = tcg_invert_cond(cond);
> +    TCGLabel *l_exit;
> +    TCGv t0, t1;
> +
> +    l_exit = gen_new_label();
> +
> +    t0 = tcg_temp_new();
> +    gen_load_spr(t0, reg);
> +    tcg_gen_andi_tl(t0, t0, mask);
> +    tcg_gen_brcondi_tl(exit_cond, t0, mask, l_exit);

When testing a single bit, compare against 0, not the bit.

> +    t1 = tcg_temp_new();
> +    gen_load_spr(t1, spr);
> +    tcg_gen_addi_tl(t1, t1, inc_val);
> +    gen_store_spr(spr, t1);

It will probably perform better to make this a true conditional add.
I.e.

    gen_load_spr(t0, spr);
    tcg_gen_addi_tl(t1, t0, inc);
    tcg_gen_movcond_tl(cond, t0, reg, zero, t1, t0);
    gen_store_spr(spr, t0);

> -    /*
> -     * The PMU insns_inc() helper stops the internal PMU timer if a
> -     * counter overflows happens. In that case, if the guest is
> -     * running with icount and we do not handle it beforehand,
> -     * the helper can trigger a 'bad icount read'.
> -     */
> -    gen_icount_io_start(ctx);

Removing this is incorrect.

> -    gen_helper_insns_inc(cpu_env, tcg_constant_i32(ctx->base.num_insns));
> +    inc_spr_if_cond(SPR_POWER_MMCR0, MMCR0_FC56, TCG_COND_NE,
> +                    SPR_POWER_PMC5, ctx->base.num_insns);

This is non-bisectable.  You're removing support for all registers and only adding back 
PMC5.  You add them all back before the end of the series, but the middle patches do not 
behave correctly.


r~