Re: [PATCH 12/19] target/ppc/pmu_book3s_helper.c: enable PMC1 counter negative EBB

[Richard Henderson]

On 8/12/21 11:24 AM, Daniel Henrique Barboza wrote:
> +void helper_insns_inc(CPUPPCState *env)
> +{
> +    env->pmu_insns_count++;
> +}
> +
> +void helper_insns_dec(CPUPPCState *env)
> +{
> +    env->pmu_insns_count--;
> +}
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 60f35360b7..c56c656694 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -8689,6 +8689,7 @@ static void ppc_tr_tb_start(DisasContextBase *db, 
> CPUState *cs)
>
>   static void ppc_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
>   {
> +    gen_helper_insns_inc(cpu_env);
>       tcg_gen_insn_start(dcbase->pc_next);
>   }
>
> @@ -8755,6 +8756,8 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, 
> CPUState *cs)
>           return;
>       }
>
> +    gen_helper_insns_dec(cpu_env);
> +
>       /* Honor single stepping. */
>       sse = ctx->singlestep_enabled & (CPU_SINGLE_STEP | GDBSTUB_SINGLE_STEP);
>       if (unlikely(sse)) {
>
>
> And then I used 'env->pmu_insns_count' to update the instruction counters. And 
> it's
> working, with a slightly better performance than we have with the icount()
> version. I'm not sure why it's working now since I tried something very similar
> before and it didn't. Figures.

Not sure why you're decrementing there.

Also, how do you want to count retired instructions? Ones that merely start?  E.g. 
including loads that fault?  How about illegal instructions?  Or does the instruction need 
to run to completion?  Which of these you choose affects where you place the increment.

It is of course extremely heavyweight to call a helper to perform a simple 
addition operation.

You may also wish to look at target/hexagon, gen_exec_counters(), which is doing the same 
sort of thing.  But not completely, since it loses count along dynamic exception paths 
(e.g. faulting load).  That can be fixed as well, via restore_state_to_opc.


r~