Re: [Qemu-devel] [PATCH 4/7] tcg: Add support for "inlining" regions of code

[Richard Henderson]

On 09/26/2017 09:31 AM, Lluís Vilanova wrote:
> Lluís Vilanova writes:
> 
>> Richard Henderson writes:
>>> On 09/14/2017 08:20 AM, Lluís Vilanova wrote:
>>>> Richard Henderson writes:
>>>>
>>>>> On 09/10/2017 09:27 AM, Lluís Vilanova wrote:
>>>>>> TCG BBLs and instructions have multiple exit points from where to raise
>>>>>> tracing events, but some of the necessary information in the generic
>>>>>> disassembly infrastructure is not available until after generating these
>>>>>> exit points.
>>>>>>
>>>>>> This patch adds support for "inline points" (where the tracing code will
>>>>>> be placed), and "inline regions" (which identify the TCG code that must
>>>>>> be inlined). The TCG compiler will basically copy each inline region to
>>>>>> any inline points that reference it.
>>>>
>>>>> I am not keen on this.
>>>>
>>>>> Is there a reason you can't just emit the tracing code at the appropriate 
>>>>> place
>>>>> to begin with?  Perhaps I have to wait to see how this is used...
>>>>
>>>> As I tried to briefly explain on next patch, the main problem without 
>>>> inlining
>>>> is that we will see guest_tb_after_trans twice on the trace for each TB in
>>>> conditional instructions on the guest, since they have two exit points 
>>>> (which we
>>>> capture when emitting goto_tb in TCG).
> 
>>> Without seeing the code, I suspect this is because you didn't examine the
>>> argument to tcg_gen_exit_tb.  You can tell when goto_tb must have been 
>>> emitted
>>> and avoid logging twice.
> 
>> The generated tracing code for 'guest_*_after' must be right before the
>> "goto_tb" opcode at the end of a TB (AFAIU generated by
>> tcg_gen_lookup_and_goto_ptr()), and we have two of those when decoding a 
>> guest
>> conditional jump.
> 
>> If we couple this with the semantics of the trace_*_tcg functions (trace the
>> event at translation time, and generate TCG code to trace the event at 
>> execution
>> time), we get the case I described (we don't want to call 
>> trace_tb_after_tcg()
>> or trace_insn_after_tcg() twice for the same TB or instruction).
> 
>> That is, unless I've missed something.
> 
> 
>> The only alternative I can think of is changing tracetool to offer an 
>> additional
>> API that provides separate functions for translation-time tracing and
>> execution-time generation. So from this:
> 
>>   static inline void trace_event_tcg(CPUState *cpu, TCGv_env env, ...)
>>   {
>>       trace_event_trans(cpu, ...);
>>       if (trace_event_get_vcpu_state(cpu, EVENT_EXEC)) {
>>           gen_helper_trace_event_exec(env, ...);
>>       }
>>   }
> 
>> We can extend it into this:
> 
>>   static inline void gen_trace_event_exec(TCGv_env env, ...)
>>       if (trace_event_get_vcpu_state(cpu, EVENT_EXEC)) {
>>           gen_helper_trace_event_exec(env, ...);
>>       }
>>   }
>>   static inline void trace_event_tcg(CPUState *cpu, TCGv_env env, ...)
>>   {
>>       trace_event_trans(cpu, ...);
>>       gen_trace_event_exec(env, ...);
>>   }
> 
> Richard, do you prefer to keep the "TCG inline" feature or switch the internal
> tracing API to this second approach?

I don't think I fully understand what you're proposing.  The example
transformation above is merely syntactic and has no functional change.

As previously stated, I'm not keen on the "tcg inline" approach.  I would
prefer that you hook into tcg_gen_{exit_tb,goto_tb,goto_ptr} functions within
tcg/tcg-op.c to log transitions between TBs.


r~