Re: [Qemu-devel] [RFC][PATCH v2 3/4] tcg: add optimized TCG qemu_ld/st generation

[Yeongkyoon Lee <yeongkyoon.lee@samsung.com>]

> Is it really worth having this as a CONFIG_ switch? If we think
> it's better to do this out of line we should just switch to
> always generating the out of line code, I think. There's not much
> point in retaining the old code path if it's disabled -- it will
> just bitrot.

I agree.
However, it is just a safe guard because I have not test all the targets 
of qemu.
I've only tested x86 and ARM targets on x86 and x86-64 hosts.
If agreed to remove conditional macro, then I'll fix it.


>> +#ifdef CONFIG_QEMU_LDST_OPTIMIZATION
>> +    /* jne slow_path */
>> +    tcg_out_opc(s, OPC_JCC_long + JCC_JNE, 0, 0, 0);
>> +    if (!label_ptr) {
>> +        tcg_abort();
>> +    }
> There's no point in this check and abort -- label_ptr will always be 
> non-NULL (it would be an internal error if it wasn't), and if it is by 
> some future bug NULL, we'll just crash on the next line, which is just 
> as good. The existing code didn't feel the need to make this check, we 
> don't need to do it in the new code. 

It cannot be happened now as you said. It is just for a possible future bug.
But I cannot understand "we'll just crash on the next line" you 
mentioned above.

>> +#if defined(CONFIG_QEMU_LDST_OPTIMIZATION)
>> +    /* helper stub will be jumped back here */
> "will jump back here".

Ok.

>
>> +#if defined(CONFIG_QEMU_LDST_OPTIMIZATION)
>> +    /* helper stub will be jumped back here */
> ditto.

Ok.

>> +#if defined(CONFIG_QEMU_LDST_OPTIMIZATION)
>> +/* optimization to reduce jump overheads for qemu_ld/st IRs */
>> +
>> +/*
>> + * qemu_ld/st code generator call add_qemu_ldst_label,
>> + * so that slow case(TLB miss or I/O rw) is handled at the end of TB
>> + */
> This comment isn't really describing the purpose of this function,
> which is something more along the lines of "Record the context of
> a call to the out of line helper code for the slow path for a
> load or store, so that we can later generate the correct helper
> code".

I agree. Your description looks better.

>
>> +    if (s->nb_qemu_ldst_labels >= TCG_MAX_QEMU_LDST)
>> +        tcg_abort();
> QEMU coding style requires braces. Please use checkpatch.pl.

Ok.

>
>> +
>> +    idx = s->nb_qemu_ldst_labels++;
>> +    label = (TCGLabelQemuLdst *)&s->qemu_ldst_labels[idx];
>> +    label->opc_ext = opc_ext;
>> +    label->datalo_reg = data_reg;
>> +    label->datahi_reg = data_reg2;
>> +    label->addrlo_reg = addrlo_reg;
>> +    label->addrhi_reg = addrhi_reg;
>> +    label->mem_index = mem_index;
>> +    label->raddr = raddr;
>> +    if (!label_ptr) {
>> +        tcg_abort();
>> +    }
> Another pointless abort.

ditto.

>
>> diff --git a/tcg/tcg.c b/tcg/tcg.c
>> index 8386b70..8009069 100644
>> --- a/tcg/tcg.c
>> +++ b/tcg/tcg.c
>> @@ -301,6 +301,14 @@ void tcg_func_start(TCGContext *s)
>>
>>       gen_opc_ptr = gen_opc_buf;
>>       gen_opparam_ptr = gen_opparam_buf;
>> +#if defined(CONFIG_QEMU_LDST_OPTIMIZATION)
>> +    /* initialize qemu_ld/st labels which help to generate TLB miss case codes at the end of TB */
>> +    s->qemu_ldst_labels = tcg_malloc(sizeof(TCGLabelQemuLdst) * TCG_MAX_QEMU_LDST);
>> +    if (!s->qemu_ldst_labels) {
>> +        tcg_abort();
>> +    }
> Unnecessary check -- tcg_malloc() can never return 0.

Ok.

>> +#if defined(CONFIG_QEMU_LDST_OPTIMIZATION)
>> +/* Macros and structures for qemu_ld/st IR code optimization:
>> +   It looks good for TCG_MAX_HELPER_LABELS to be half of OPC_BUF_SIZE in exec-all.h. */
>> +#define TCG_MAX_QEMU_LDST       320
> Is that true even if you have a huge block with nothing but simple
> guest load instructions in it?

I agree. It needs to be set as same size with OPC_BUF_SIZE for covering 
extreme cases.