Re: [Qemu-devel] [PATCH v2 3/6] Do constant folding for basic arithmetic operations.

[Richard Henderson]

On 06/09/2011 03:45 AM, Kirill Batuzov wrote:
> +static int op_to_mov(int op)
> +{
> +    if (op_bits(op) == 32) {
> +        return INDEX_op_mov_i32;
> +    }
> +#if TCG_TARGET_REG_BITS == 64
> +    if (op_bits(op) == 64) {
> +        return INDEX_op_mov_i64;
> +    }
> +#endif
> +    tcg_abort();
> +}

Again, switch not two calls.

> +static TCGArg do_constant_folding(int op, TCGArg x, TCGArg y)
> +{
> +    TCGArg res = do_constant_folding_2(op, x, y);
> +#if TCG_TARGET_REG_BITS == 64
> +    if (op_bits(op) == 32) {
> +        res &= 0xffffffff;

Strictly speaking, this isn't required.  The top bits of any
constant are considered garbage to the code generators.  C.f.
the code in tcg_out_movi for any 64-bit host.

That said, only x86_64 and s390 get this right for the constraints.
x86_64 by being able to use 'i' to accept all constants for 32-bit
operations, and s390 by using 'W' as a modifier to force 32-bit
comparison in tcg_target_const_match.

So it's probably best to keep this for now.

> +        /* Simplify expression if possible. */
> +        switch (op) {
> +        case INDEX_op_add_i32:
> +#if TCG_TARGET_REG_BITS == 64
> +        case INDEX_op_add_i64:
> +#endif
> +            if (temps[args[1]].state == TCG_TEMP_CONST) {
> +                /* 0 + x == x + 0 */
> +                tmp = args[1];
> +                args[1] = args[2];
> +                args[2] = tmp;
> +            }

Probably best to break this out into another switch so that
you can handle all of the commutative operations.

> +            /* Fallthrough */
> +        case INDEX_op_sub_i32:
> +#if TCG_TARGET_REG_BITS == 64
> +        case INDEX_op_sub_i64:
> +#endif
> +            if (temps[args[1]].state == TCG_TEMP_CONST) {
> +                /* Proceed with possible constant folding. */
> +                break;
> +            }
> +            if (temps[args[2]].state == TCG_TEMP_CONST
> +                && temps[args[2]].val == 0) {
> +                if ((temps[args[0]].state == TCG_TEMP_COPY
> +                    && temps[args[0]].val == args[1])
> +                    || args[0] == args[1]) {
> +                    args += 3;
> +                    gen_opc_buf[op_index] = INDEX_op_nop;
> +                } else {
> +                    reset_temp(temps, args[0], nb_temps, nb_globals);
> +                    if (args[1] >= s->nb_globals) {
> +                        temps[args[0]].state = TCG_TEMP_COPY;
> +                        temps[args[0]].val = args[1];
> +                        temps[args[1]].num_copies++;
> +                    }
> +                    gen_opc_buf[op_index] = op_to_mov(op);
> +                    gen_args[0] = args[0];
> +                    gen_args[1] = args[1];
> +                    gen_args += 2;
> +                    args += 3;
> +                }
> +                continue;
> +            }
> +            break;
> +        case INDEX_op_mul_i32:
> +#if TCG_TARGET_REG_BITS == 64
> +        case INDEX_op_mul_i64:
> +#endif
> +            if ((temps[args[1]].state == TCG_TEMP_CONST
> +                 && temps[args[1]].val == 0)
> +                || (temps[args[2]].state == TCG_TEMP_CONST
> +                    && temps[args[2]].val == 0)) {
> +                reset_temp(temps, args[0], nb_temps, nb_globals);
> +                temps[args[0]].state = TCG_TEMP_CONST;
> +                temps[args[0]].val = 0;
> +                assert(temps[args[0]].num_copies == 0);
> +                gen_opc_buf[op_index] = op_to_movi(op);
> +                gen_args[0] = args[0];
> +                gen_args[1] = 0;
> +                args += 3;
> +                gen_args += 2;
> +                continue;
> +            }

This is *way* too much code duplication, particularly with the
same code sequences already existing for mov and movi and more
to come with the logicals.


r~