On Wed, Jul 12, 2017 at 16:10:15 +0100, Alex Bennée wrote:
> Emilio G. Cota <c...@braap.org> writes:
(snip)
> > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> > index fd20bca..673b26d 100644
> > --- a/include/exec/exec-all.h
> > +++ b/include/exec/exec-all.h
> > @@ -320,14 +320,25 @@ struct TranslationBlock {
> > uint16_t size; /* size of target code for this block (1 <=
> > size <= TARGET_PAGE_SIZE) */
> > uint16_t icount;
> > - uint32_t cflags; /* compile flags */
> > + /*
> > + * @tc_size must be kept right after @tc_ptr to facilitate TB lookups
> > in a
> > + * binary search tree -- see struct ptr_size.
> > + * We use an anonymous struct here to avoid updating all calling code,
> > + * which would be quite a lot of churn.
> > + * The only reason to bring @cflags into the anonymous struct is to
> > + * avoid inducing a hole in TranslationBlock.
> > + */
> > + struct {
> > + void *tc_ptr; /* pointer to the translated code */
> > + uint32_t tc_size; /* size of translated code for this block */
> > +
> > + uint32_t cflags; /* compile flags */
> > #define CF_COUNT_MASK 0x7fff
> > #define CF_LAST_IO 0x8000 /* Last insn may be an IO access. */
> > #define CF_NOCACHE 0x10000 /* To be freed after execution */
> > #define CF_USE_ICOUNT 0x20000
> > #define CF_IGNORE_ICOUNT 0x40000 /* Do not generate icount code */
> > -
> > - void *tc_ptr; /* pointer to the translated code */
> > + };
>
> Why not just have a named structure for this so there isn't ambiguity
> between struct ptrsize and this thing.
Yeah I did v2 of this patch yesterday. Turns out using an intermediate
struct here (and in the comparison code) doesn't end up in as much churn
as I expected, so I went with that.
(snip)
> > @@ -827,16 +853,7 @@ void tb_free(TranslationBlock *tb)
> > {
> > assert_tb_locked();
> >
> > - /* In practice this is mostly used for single use temporary TB
> > - Ignore the hard cases and just back up if this TB happens to
> > - be the last one generated. */
> > - if (tcg_ctx.tb_ctx.nb_tbs > 0 &&
> > - tb == tcg_ctx.tb_ctx.tbs[tcg_ctx.tb_ctx.nb_tbs - 1]) {
> > - size_t struct_size = ROUND_UP(sizeof(*tb), qemu_icache_linesize);
> > -
> > - tcg_ctx.code_gen_ptr = tb->tc_ptr - struct_size;
> > - tcg_ctx.tb_ctx.nb_tbs--;
> > - }
> > + g_tree_remove(tcg_ctx.tb_ctx.tb_tree, &tb->tc_ptr);
> > }
>
> This function should be renamed as we never attempt to free (and it was
> pretty half hearted before). Maybe tb_remove or tb_deref?
Good point. I like tb_remove.
Emilio
