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

[Yeongkyoon Lee]

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