Re: [PATCH v2 21/22] tcg/tci: Split out constraint sets to tcg-target-con-set.h

[Peter Maydell]

On Fri, 15 Jan 2021 at 21:23, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This requires finishing the conversion to tcg_target_op_def.
> Remove quite a lot of ifdefs, since we can reference opcodes
> even if they are not implemented.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

This one's a lot more painful to review than the native targets :-(

> ---

> -/* TODO: documentation. */
> -static const TCGTargetOpDef tcg_target_op_defs[] = {
> -    { INDEX_op_exit_tb, { NULL } },
> -    { INDEX_op_goto_tb, { NULL } },
> -    { INDEX_op_br, { NULL } },

I don't see any cases in the new code for these ops,
or for INDEX_op_mb which has {}. Is the function in fact
never called for those ops ?

> +    case INDEX_op_div_i32:
> +    case INDEX_op_div_i64:
> +    case INDEX_op_divu_i32:
> +    case INDEX_op_divu_i64:
> +    case INDEX_op_rem_i32:
> +    case INDEX_op_rem_i64:
> +    case INDEX_op_remu_i32:
> +    case INDEX_op_remu_i64:
> +        return C_O1_I2(r, r, r);
>
> -    { INDEX_op_add_i32, { R, RI, RI } },
> -    { INDEX_op_sub_i32, { R, RI, RI } },
> -    { INDEX_op_mul_i32, { R, RI, RI } },
> -#if TCG_TARGET_HAS_div_i32
> -    { INDEX_op_div_i32, { R, R, R } },
> -    { INDEX_op_divu_i32, { R, R, R } },
> -    { INDEX_op_rem_i32, { R, R, R } },
> -    { INDEX_op_remu_i32, { R, R, R } },
> -#elif TCG_TARGET_HAS_div2_i32
> -    { INDEX_op_div2_i32, { R, R, "0", "1", R } },
> -    { INDEX_op_divu2_i32, { R, R, "0", "1", R } },
> -#endif

Why don't we need all the ifdeffery the old code has ? Is
it because we know the ifdefs are always true (or always false) ?
If so, can we do the "drop ifdefs" in a separate patch beforehand?
I think that might help make the patch a bit easier to review.

thanks
-- PMM