Re: [PATCH 04/17] target/i386: add ALU load/writeback core

[Richard Henderson]

On 8/24/22 10:31, Paolo Bonzini wrote:
> Add generic code generation that takes care of preparing operands
> around calls to decode.e.gen in a table-driven manner, so that ALU
> operations need not take care of that.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/tcg/decode-new.c.inc | 14 +++++++-
>   target/i386/tcg/emit.c.inc       | 62 ++++++++++++++++++++++++++++++++
>   2 files changed, 75 insertions(+), 1 deletion(-)
>
> diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc
> index d661f1f6f0..b53afea9c8 100644
> --- a/target/i386/tcg/decode-new.c.inc
> +++ b/target/i386/tcg/decode-new.c.inc
> @@ -133,6 +133,7 @@ typedef struct X86DecodedOp {
>       MemOp ot;     /* For b/c/d/p/s/q/v/w/y/z */
>       X86ALUOpType alu_op_type;
>       bool has_ea;
> +    TCGv v;
>   } X86DecodedOp;
>   
>   struct X86DecodedInsn {
> @@ -987,7 +988,18 @@ static target_ulong disas_insn_new(DisasContext *s, 
> CPUState *cpu, int b)
>       if (decode.op[0].has_ea || decode.op[1].has_ea || decode.op[2].has_ea) {
>           gen_load_ea(s, &decode.mem);
>       }
> -    decode.e.gen(s, env, &decode);
> +    if (s->prefix & PREFIX_LOCK) {
> +        if (decode.op[0].alu_op_type != X86_ALU_MEM) {
> +            goto illegal_op;
> +        }
> +        gen_load(s, s->T1, &decode.op[2], decode.immediate);
> +        decode.e.gen(s, env, &decode);
> +    } else {
> +        gen_load(s, s->T0, &decode.op[1], decode.immediate);
> +        gen_load(s, s->T1, &decode.op[2], decode.immediate);
> +        decode.e.gen(s, env, &decode);
> +        gen_writeback(s, &decode.op[0]);
> +    }

While I can see that you don't want to mix decoding changes with temp lifetime 
changes...

> +static void gen_load(DisasContext *s, TCGv v, X86DecodedOp *op, uint64_t imm)
> +{
...
> +    op->v = v;
> +}

Surely this assignment...

> +static void gen_writeback(DisasContext *s, X86DecodedOp *op)
> +{
...
> +    case X86_ALU_GPR:
> +        gen_op_mov_reg_v(s, op->ot, op->n, s->T0);

... can be used here instead of hard-coding T0.  It should be easy enough to create *_v 
editions of all *_T0, such as gen_movl_seg_T0.



r~