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

[Richard Henderson <rth@twiddle.net>]

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.

>        }
>    }
>
> 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.

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


r~