Re: [PATCH for-9.1 11/19] target/i386: move C0-FF opcodes to new decoder (except for x87)

[Richard Henderson]

On 4/9/24 06:43, Paolo Bonzini wrote:
> diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
> index 05a1912f8a3..88653c4f824 100644
> --- a/include/tcg/tcg.h
> +++ b/include/tcg/tcg.h
> @@ -105,6 +105,12 @@ typedef uint64_t TCGRegSet;
>   /* Turn some undef macros into true macros.  */
>   #define TCG_TARGET_HAS_add2_i32         1
>   #define TCG_TARGET_HAS_sub2_i32         1
> +/* Define parameterized _tl macros.  */
> +#define TCG_TARGET_deposit_tl_valid     TCG_TARGET_deposit_i32_valid
> +#define TCG_TARGET_extract_tl_valid     TCG_TARGET_extract_i32_valid
> +#else
> +#define TCG_TARGET_deposit_tl_valid     TCG_TARGET_deposit_i64_valid
> +#define TCG_TARGET_extract_tl_valid     TCG_TARGET_extract_i64_valid
>   #endif

So far we have been localizing these to emit.c.inc.

In general I'm not sure how I feel about them.  It would be cleaner not to expose tcg 
backend details at all, but there are several points where we can legitimately produce 
better code knowing what the backend has, without having to have a strong optimizer.


> +static void decode_group2(DisasContext *s, CPUX86State *env, X86OpEntry 
> *entry, uint8_t *b)
> +{
> +    static const X86GenFunc group2_gen[8] = {
> +        gen_ROL, gen_ROR, gen_RCL, gen_RCR, gen_SHL, gen_SHR, gen_SHL, gen_SAR,
> +    };

I think you need to keep a comment for /6 (currently OP_SHL1 /* undocumented */; I presume 
this to be the original SAL opcode, before some prehistoric documentation changed it to 
share /4 with SHL).

> +static const X86OpEntry opcodes_grp3[16] = {
> +    /* 0xf6 */
> +    [0x00] = X86_OP_ENTRYrr(AND, E,b, I,b),
> +    [0x02] = X86_OP_ENTRY1(NOT,  E,b,      lock),
> +    [0x03] = X86_OP_ENTRY1(NEG,  E,b,      lock),
> +    [0x04] = X86_OP_ENTRYrr(MUL, E,b, 0,b, zextT0),
> +    [0x05] = X86_OP_ENTRYrr(IMUL,E,b, 0,b, sextT0),
> +    [0x06] = X86_OP_ENTRYr(DIV,  E,b),
> +    [0x07] = X86_OP_ENTRYr(IDIV, E,b),
> +
> +    /* 0xf7 */
> +    [0x08] = X86_OP_ENTRYrr(AND, E,v, I,z),
> +    [0x0a] = X86_OP_ENTRY1(NOT,  E,v,      lock),
> +    [0x0b] = X86_OP_ENTRY1(NEG,  E,v,      lock),
> +    [0x0c] = X86_OP_ENTRYrr(MUL, E,v, 0,v, zextT0),
> +    [0x0d] = X86_OP_ENTRYrr(IMUL,E,v, 0,v, sextT0),
> +    [0x0e] = X86_OP_ENTRYr(DIV,  E,v),
> +    [0x0f] = X86_OP_ENTRYr(IDIV, E,v),
> +};
> +
> +static void decode_group3(DisasContext *s, CPUX86State *env, X86OpEntry 
> *entry, uint8_t *b)
> +{
> +    int w = (*b & 1);
> +    int reg = (get_modrm(s, env) >> 3) & 7;
> +
> +    *entry = opcodes_grp3[(w << 3) | reg];
> +}
> +
> +static const X86OpEntry opcodes_grp4[16] = {
> +    /* 0xfe */
> +    [0x00] = X86_OP_ENTRY1(INC,     E,b, lock),
> +    [0x01] = X86_OP_ENTRY1(DEC,     E,b, lock),
> +
> +    /* 0xff */
> +    [0x08] = X86_OP_ENTRY1(INC,     E,v, lock),
> +    [0x09] = X86_OP_ENTRY1(DEC,     E,v, lock),
> +    [0x0a] = X86_OP_ENTRY3(CALL_m,  None, None, E,f64, None, None, zextT0),
> +    [0x0b] = X86_OP_ENTRYr(CALLF_m, M,p),
> +    [0x0c] = X86_OP_ENTRY3(JMP_m,   None, None, E,f64, None, None, zextT0),
> +    [0x0d] = X86_OP_ENTRYr(JMPF_m,  M,p),
> +    [0x0e] = X86_OP_ENTRYr(PUSH,    E,f64),
> +};
> +
> +static void decode_group4(DisasContext *s, CPUX86State *env, X86OpEntry 
> *entry, uint8_t *b)
> +{
> +    int w = (*b & 1);
> +    int reg = (get_modrm(s, env) >> 3) & 7;
> +
> +    *entry = opcodes_grp4[(w << 3) | reg];
> +}

Did these tables need to be outside their functions?
Though this works, 0xff is named grp5.

> +    [0xF1] = X86_OP_ENTRY0(INT1,   svm(ICEBP)),
> +    [0xF4] = X86_OP_ENTRY0(HLT,    chk(cpl0)),
> +    [0xF5] = X86_OP_ENTRY0(CMC),
> +    [0xF6] = X86_OP_GROUP1(group3, E,b),
> +    [0xF7] = X86_OP_GROUP1(group3, E,v),

Not adding spacers as you were above?

> +static void gen_RCL(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
> +{
> +    bool have_1bit_cin, can_be_zero;
> +    TCGv count;
> +    TCGLabel *zero_label = NULL;
> +    MemOp ot = gen_shift_count(s, decode, &can_be_zero, &count);
> +    TCGv low = tcg_temp_new();
> +    TCGv high = tcg_temp_new();
> +    TCGv low_count = tcg_temp_new();
> +
> +    if (!count) {
> +        return;
> +    }
> +

Delay all temp allocation until after the early return.


> +static void gen_rot_carry(X86DecodedInsn *decode, TCGv result, TCGv count, int 
> bit)
> +{
> +    TCGv temp = count ? tcg_temp_new() : decode->cc_dst;
> +
> +    tcg_gen_setcondi_tl(TCG_COND_TSTNE, temp, result, 1ULL << bit);

tcg_gen_extract_tl.

It might be easier to read with only one if:

    if (count == NULL) {
        extract
    } else {
        temp = new
        extract
        movcond
    }


r~