Re: [Qemu-devel] [PATCH RFC 2/3] tcg/optimize: do copy propagation for memory locations

[Kirill Batuzov]

On Wed, 22 Nov 2017, Richard Henderson wrote:

> On 11/09/2017 03:41 PM, Kirill Batuzov wrote:
> > +typedef struct TCGMemLocation {
> > +    /* Offset is relative to ENV. Only fields of CPUState are accounted.  
> > */
> > +    tcg_target_ulong offset;
> > +    tcg_target_ulong size;
> > +    TCGType type;
> > +    /* Pointer to a temp containing a valid copy of this memory location.  
> > */
> > +    TCGTemp *copy;
> > +    /* Pointer to the next memory location containing copy of the same
> > +       content.  */
> > +    struct TCGMemLocation *next_copy;
> 
> Did you ever find copies of memories that weren't also copies within temps?
> I.e. you could have found this through copy->next_copy?

Yes. This happens when a temp was stored to multiple memory locations.

> 
> > +    /* Double-linked list of all memory locations.  */
> > +    struct TCGMemLocation *next;
> > +    struct TCGMemLocation **prev_ptr;
> 
> Use QTAILQ_* for common double-linked-list manipulation.
> 
> > +struct TCGMemLocation *mem_locations;
> > +struct TCGMemLocation *free_mls;
> 
> These can't be globals anymore -- we're do multi-threaded code generation now.

Then they should be moved to TCGContext I assume?

> 
> > @@ -77,12 +125,27 @@ static void reset_ts(TCGTemp *ts)
> >      struct tcg_temp_info *pi = ts_info(ti->prev_copy);
> >      struct tcg_temp_info *ni = ts_info(ti->next_copy);
> >  
> > +    if (ti->mem_loc && ts_is_copy(ts) && 0) {
> > +        TCGMemLocation *ml, *nml;
> > +        for (ml = ti->mem_loc; ml; ml = nml) {
> > +            nml = ml->next_copy;
> > +            ml->copy = ti->next_copy;
> > +            ml->next_copy = ni->mem_loc;
> > +            ni->mem_loc = ml;
> > +        }
> > +    } else {
> > +        while (ti->mem_loc) {
> > +            reset_ml(ti->mem_loc);
> > +        }
> 
> Why would a single temp be associated with more than one memory?
>

Because it was stored to multiple memory locations. And when reading from
any of these locations we want to access the temp instead.

For example this happens when we translate ARM32 VDUP instruction. One
value is duplicated into all elements of the vector. When elements of the
vector are accessed later, we want to use this value instead of rereading
it from memory.

> > +static TCGOpcode ld_to_mov(TCGOpcode op)
> > +{
> > +#define LD_TO_EXT(sz, w)                                 \
> > +    case glue(glue(INDEX_op_ld, sz), glue(_i, w)):       \
> > +        return glue(glue(INDEX_op_ext, sz), glue(_i, w))
> > +
> > +    switch (op) {
> > +    LD_TO_EXT(8s, 32);
> > +    LD_TO_EXT(8u, 32);
> > +    LD_TO_EXT(16s, 32);
> > +    LD_TO_EXT(16u, 32);
> > +    LD_TO_EXT(8s, 64);
> > +    LD_TO_EXT(8u, 64);
> > +    LD_TO_EXT(16s, 64);
> > +    LD_TO_EXT(16u, 64);
> > +    LD_TO_EXT(32s, 64);
> > +    LD_TO_EXT(32u, 64);
> 
> How many extensions did you find?  Or is this Just In Case?
> 

One. It was in Aartch64 build of x264. So this is more Just in Case. But
it may become useful if we try to emulate some 8- or 16-bit architectures.

-- 
Kirill