Re: [PATCH v12 02/15] accel: collecting TB execution count

[Wu, Fei]

On 5/25/2023 1:02 AM, Richard Henderson wrote:
> On 5/24/23 06:35, Wu, Fei wrote:
>> On 5/23/2023 8:45 AM, Richard Henderson wrote:
>>> On 5/18/23 06:57, Fei Wu wrote:
>>>> +void HELPER(inc_exec_freq)(void *ptr)
>>>> +{
>>>> +    TBStatistics *stats = (TBStatistics *) ptr;
>>>> +    tcg_debug_assert(stats);
>>>> +    ++stats->executions.normal;
>>>> +}
>>> ...
>>>> +static inline void gen_tb_exec_count(TranslationBlock *tb)
>>>> +{
>>>> +    if (tb_stats_enabled(tb, TB_EXEC_STATS)) {
>>>> +        TCGv_ptr ptr = tcg_temp_new_ptr();
>>>> +        tcg_gen_movi_ptr(ptr, (intptr_t)tb->tb_stats);
>>>> +        gen_helper_inc_exec_freq(ptr);
>>>> +    }
>>>> +}
>>>
>>> This is 3 host instructions, easily expanded inline:
>>>
>>> --- a/accel/tcg/translator.c
>>> +++ b/accel/tcg/translator.c
>>> @@ -11,6 +11,7 @@
>>>   #include "qemu/error-report.h"
>>>   #include "tcg/tcg.h"
>>>   #include "tcg/tcg-op.h"
>>> +#include "tcg/tcg-temp-internal.h"
>>>   #include "exec/exec-all.h"
>>>   #include "exec/gen-icount.h"
>>>   #include "exec/log.h"
>>> @@ -18,6 +19,30 @@
>>>   #include "exec/plugin-gen.h"
>>>   #include "exec/replay-core.h"
>>>
>>> +
>>> +static void gen_tb_exec_count(TranslationBlock *tb)
>>> +{
>>> +    if (tb_stats_enabled(tb, TB_EXEC_STATS)) {
>>> +        TCGv_ptr ptr = tcg_temp_ebb_new_ptr();
>>> +
>>> +        tcg_gen_movi_ptr(ptr,
>>> (intptr_t)&tb->tb_stats->executions.normal);
>>> +        if (sizeof(tb->tb_stats->executions.normal) == 4) {
>>> +            TCGv_i32 t = tcg_temp_ebb_new_i32();
>>> +            tcg_gen_ld_i32(t, ptr, 0);
>>> +            tcg_gen_addi_i32(t, t, 1);
>>> +            tcg_gen_st_i32(t, ptr, 0);
>>> +            tcg_temp_free_i32(t);
>>> +        } else {
>>> +            TCGv_i64 t = tcg_temp_ebb_new_i64();
>>> +            tcg_gen_ld_i64(t, ptr, 0);
>>> +            tcg_gen_addi_i64(t, t, 1);
>>> +            tcg_gen_st_i64(t, ptr, 0);
>>> +            tcg_temp_free_i64(t);
>>> +        }
>>> +        tcg_temp_free_ptr(ptr);
>>> +    }
>>> +}
>>> +
>>>   bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest)
>>>   {
>>>       /* Suppress goto_tb if requested. */
>>>
>>>
>>> I'm not expecially keen on embedding the TBStatistics pointer directly
>>> like this; for most hosts we will have to put this constant into the
>>> constant pool.  Whereas the pointer already exists at tb->tb_stats, and
>>> tb is at a constant displacement prior to the code, so we already have
>>> mechanisms for generating pc-relative addresses.
>>>
>>> However, that's premature optimization.  Let's get it working first.
>>>
>> Richard, have you reviewed the whole series? I will integrate your
>> change to next version.
> 
> No, it's difficult to see what's going on.
> In your next revision, please remove CONFIG_PROFILER entirely first,
> which was what I was planning to do locally.
> 
OK, I will update it.

Thanks,
Fei.

> 
> r~