Re: [Qemu-devel] [RFC] [tcg] Idea on refactoring target code generation loop (gen_intermediate_code)

[Lluís Vilanova]

Richard Henderson writes:

> On 03/09/2016 01:16 PM, Lluís Vilanova wrote:
>> Richard Henderson writes:
>> 
>>> On 03/09/2016 09:38 AM, Lluís Vilanova wrote:
>>>> Hi,
>>>> 
>>>> NOTE: I won't be throwing patches anytime soon, I just want to know if 
>>>> there's
>>>> interest in this for the future.
>>>> 
>>>> While adding events for tracing guest instructions, I've found that the
>>>> per-target "gen_intermediate_code()" function is very similar but not 
>>>> exactly
>>>> the same for each of the targets. This makes architecture-agnostic features
>>>> harder to maintain across targets, specially when it comes to their 
>>>> relative
>>>> order.
>>>> 
>>>> So, would it be worth it if I generalized part of that code into an
>>>> architecture-agnostic function that calls into target-specific hooks 
>>>> wherever it
>>>> needs extending? There are many ways to do it that we can discuss later.
>> 
>>> It's worth talking about, since I do believe it would make long-term 
>>> maintenance
>>> across the targets easier.
>> 
>>> These "target-specific hooks" probably ought not be "hooks" in the
>>> traditional sense of attaching them to CPUState.  I'd be more comfortable 
>>> with a
>>> refactoring that used include files -- maybe .h or maybe .inc.c.  If we do 
>>> the
>>> normal sort of hook, then we've got to either expose DisasContext in places 
>>> we
>>> shouldn't, or dynamically allocate it.  Neither seems particularly 
>>> appealing.
>> 
>> Great. I pondered about using QOM vs "template code", and I'm leaning towards
>> the latter:
>> 
>> * translate.c:
>> 
>> struct DisasContextArch {
>> DisasContext common;
>> };
>> 
>> // implement "hooks"
>> 
>> void gen_intermediate_code(CPUArchState *env, TranslationBlock *tb)
>> {
>> DisasContextArch dc;
>> gen_intermediate_code_template(get_cpu(env), &dc.common, tb);

> Pointer down into "common" here...

>> }
>> 
>> * translate-template.h:
>> 
>> struct DisasContext { /* ... */ };
>> 
>> void gen_intermediate_code_template(CPUState *cpu, DisasContext *dc,
>> TranslationBlock *tb)
>> {
>> // init dc
>> // arch-specific init dc hook
>> 
>> // gen_icount, etc.
>> while (true) {
>> // generic processing code with calls to hooks

> ... means you have to upcast to DisasContextArch here, or in the hooks 
> themselves.

Yup, that was the intention. Hooks should then use "container_of()" to get their
part.


>> }
>> }
>> 
>> While initially simpler, the "template code" still feels a little dirty to
>> me. With QOM, you could implement a DisasContextClass that provides the
>> per-target hook pointers, which can be globally allocated once per target and
>> pointed to by the DisasContext allocated in stack.
>> 
>> I'm not sure about what you mean by exposing DisasContext in places it 
>> shouldn't
>> be, though.

> You either split DisasContextArch / DisasContext in "odd" ways, and then have 
> to
> cast back and forth, or you have to expose the whole of DisasContextArch. It's
> the latter that I considered inappropriate.

Well, moving whatever is used in the generic code into DisasContext doesn't
sound too odd as a first solution.


> Alternately... can we broach the subject of C++?  Honestly, it seems we work 
> too
> hard sometimes to re-implement templates and classes in C.

Whooo, I'd really *love* to switch to C++ just for templates and classes... But
last time this was discussed, the idea wasn't met with much joy :)


Cheers,
  Lluis