Re: [Qemu-devel] [PATCH v3 5/9] tcg: Reorg TCGOp chaining

[Richard Henderson]

On 07/25/2016 04:53 PM, Aurelien Jarno wrote:
> On 2016-06-23 20:48, Richard Henderson wrote:
> > @@ -1287,18 +1287,10 @@ void tcg_op_remove(TCGContext *s, TCGOp *op)
> >      int next = op->next;
> >      int prev = op->prev;
> >
> > -    if (next >= 0) {
> > -        s->gen_op_buf[next].prev = prev;
> > -    } else {
> > -        s->gen_last_op_idx = prev;
> > -    }
> > -    if (prev >= 0) {
> > -        s->gen_op_buf[prev].next = next;
> > -    } else {
> > -        s->gen_first_op_idx = next;
> > -    }
> > +    s->gen_op_buf[next].prev = prev;
> > +    s->gen_op_buf[prev].next = next;
> >
> > -    memset(op, -1, sizeof(*op));
> > +    memset(op, 0, sizeof(*op));
>
> Doing so means you can remove the op at gen_op_buf[0]. The doubled
> linked list is still valid, but then I guess you can't assume anymore
> that the first op is gen_op_buf[0], as it is done elsewhere your patch.
>
> I guess it's unlikely to happen that we have to remove the first op.
> Maybe adding an assert there is enough?

Assert added.

> > -    /* Index of the prex/next op, or -1 for the end of the list.  */
> > -    signed prev     : 16;
> > -    signed next     : 16;
> > +    /* Index of the prex/next op, or 0 for the end of the list.  */
>
> It's not introduced by your patch, but you might want to fix the typo
> prex -> prev.

Fixed.

> It seems that gen_first_op_idx and gen_last_op_idx are now unused.
> Shouldn't they be removed?

Yep, fixed.


r~