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

[Richard Henderson]

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?

> +    /* 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.

> @@ -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?

> +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?

Otherwise this looks quite reasonable.


r~