Re: [PATCH 03/17] target/i386: add core of new i386 decoder

[Richard Henderson]

On 8/24/22 10:31, Paolo Bonzini wrote:
> +static X86OpEntry A4_00_F7[16][8] = {

const.  Especially for the big tables, but really for anything static that you can get 
away with.

> +static void decode_threebyte_38(DisasContext *s, CPUX86State *env, X86OpEntry 
> *entry, uint8_t *b)
> +{
> +    *b = x86_ldub_code(env, s);
> +    if (!(*b & 8)) {
> +        *entry = A4_00_F7[*b >> 4][*b & 7];
> +    } else {
> +        *entry = A4_08_FF[*b >> 4][*b & 7];
> +    }
> +}

I'm not keen on the split table.
Surely it would be just as readable as

static const X86OpEntry onebyte[256] = {
    /*
     * Table A-2: One-byte Opcode Map: 00H — F7H
     */
    [0x00] = X86_OP_ENTRY2(ADD, E,b, G,b),

    [0x01] = X86_OP_ENTRY2(ADD, E,v, G,v),

    ...
    [0x10] = X86_OP_ENTRY2(ADC, E,b, G,b),
    [0x11] = X86_OP_ENTRY2(ADC, E,v, G,v),
    ...

    /*
     * Table A-2: One-byte Opcode Map: 08H - FFH
     */
    [0x08] = X86_OP_ENTRY2(OR, E,b, G,b),

    [0x09] = X86_OP_ENTRY2(OR, E,v, G,v),

    ...
    [0x0F] = { .decode = decode_twobyte, .is_decode = true },
    ...

};

> +static MemOp decode_op_size(DisasContext *s, X86OpSize size)
> +{
> +    switch (size) {
> +    case X86_SIZE_b:  /* byte */
> +        return MO_8;
> +
> +    case X86_SIZE_c:  /* 16/32-bit, based on operand size */
> +        return s->dflag == MO_64 ? MO_32 : s->dflag;

Should be 8/16.

> +static void decode_op(DisasContext *s, CPUX86State *env, X86DecodedInsn 
> *decode,
> +                      X86DecodedOp *op, X86OpType type, int b)
> +{
> +    int modrm;
> +
> +    switch (type) {
> +    case X86_TYPE_A:  /* Implicit */
> +    case X86_TYPE_F:  /* EFLAGS/RFLAGS */
> +        break;
> +
> +    case X86_TYPE_B:  /* VEX.vvvv selects a GPR */
> +        op->alu_op_type = X86_ALU_GPR;
> +        op->n = s->vex_v;
> +        break;

Validate vex prefix present, or is it intended to happen elsewhere?

> +
> +    case X86_TYPE_C:  /* REG in the modrm byte selects a control register */) 
> | REX_R(s);

Cut error, before ).


r~