Re: [Qemu-devel] [PATCH 2/5] tcg: Simplify logic using TCG_OPF_NOT_PRESENT

[Claudio Fontana <claudio.fontana@huawei.com>]

Hello Richard,

eons ago Richard wrote:
> 
> Expand the definition of "not present" to include "should not be present".
> This means we can simplify the logic surrounding the generic tcg opcodes
> for which the host backend ought not be providing definitions.
> 
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  tcg/tcg-opc.h | 26 +++++++++++++++-----------
>  tcg/tcg.c     |  4 +---
>  tcg/tcg.h     |  3 ++-
>  3 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/tcg/tcg-opc.h b/tcg/tcg-opc.h
> index db5e6e5..83f7147 100644
> --- a/tcg/tcg-opc.h
> +++ b/tcg/tcg-opc.h
> @@ -27,17 +27,21 @@
>   */
>  
>  /* predefined ops */
> -DEF(end, 0, 0, 0, 0) /* must be kept first */
> -DEF(nop, 0, 0, 0, 0)
> -DEF(nop1, 0, 0, 1, 0)
> -DEF(nop2, 0, 0, 2, 0)
> -DEF(nop3, 0, 0, 3, 0)
> -DEF(nopn, 0, 0, 1, 0) /* variable number of parameters */
> +DEF(end, 0, 0, 0, TCG_OPF_NOT_PRESENT) /* must be kept first */
> +DEF(nop, 0, 0, 0, TCG_OPF_NOT_PRESENT)
> +DEF(nop1, 0, 0, 1, TCG_OPF_NOT_PRESENT)
> +DEF(nop2, 0, 0, 2, TCG_OPF_NOT_PRESENT)
> +DEF(nop3, 0, 0, 3, TCG_OPF_NOT_PRESENT)
>  
> -DEF(discard, 1, 0, 0, 0)
> +/* variable number of parameters */
> +DEF(nopn, 0, 0, 1, TCG_OPF_NOT_PRESENT)
> +
> +DEF(discard, 1, 0, 0, TCG_OPF_NOT_PRESENT)
> +DEF(set_label, 0, 0, 1, TCG_OPF_BB_END | TCG_OPF_NOT_PRESENT)
> +
> +/* variable number of parameters */
> +DEF(call, 0, 1, 2, TCG_OPF_CALL_CLOBBER | TCG_OPF_NOT_PRESENT)

If TCG_OPF_NOT_PRESENT is supposed to mark opcodes that should not be implemented by the target, then setting TCG_OPF_NOT_PRESENT for 'call' seems wrong to me.
This will actually trigger the code in the ifdef CONFIG_DEBUG_TCG and cause an assert to fail. 

>  
> -DEF(set_label, 0, 0, 1, TCG_OPF_BB_END)
> -DEF(call, 0, 1, 2, TCG_OPF_CALL_CLOBBER) /* variable number of parameters */
>  DEF(br, 0, 0, 1, TCG_OPF_BB_END)
>  
>  #define IMPL(X) (__builtin_constant_p(X) && !(X) ? TCG_OPF_NOT_PRESENT : 0)
> @@ -166,9 +170,9 @@ DEF(muls2_i64, 2, 2, 0, IMPL64 | 
> IMPL(TCG_TARGET_HAS_muls2_i64))
>  
>  /* QEMU specific */
>  #if TARGET_LONG_BITS > TCG_TARGET_REG_BITS
> -DEF(debug_insn_start, 0, 0, 2, 0)
> +DEF(debug_insn_start, 0, 0, 2, TCG_OPF_NOT_PRESENT)
>  #else
> -DEF(debug_insn_start, 0, 0, 1, 0)
> +DEF(debug_insn_start, 0, 0, 1, TCG_OPF_NOT_PRESENT)
>  #endif
>  DEF(exit_tb, 0, 0, 1, TCG_OPF_BB_END)
>  DEF(goto_tb, 0, 0, 1, TCG_OPF_BB_END)
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index 1d8099c..c7e6567 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -1160,9 +1160,7 @@ void tcg_add_target_add_op_defs(const TCGTargetOpDef 
> *tdefs)
>      i = 0;
>      for (op = 0; op < ARRAY_SIZE(tcg_op_defs); op++) {
>          const TCGOpDef *def = &tcg_op_defs[op];
> -        if (op < INDEX_op_call
> -            || op == INDEX_op_debug_insn_start
> -            || (def->flags & TCG_OPF_NOT_PRESENT)) {
> +        if (def->flags & TCG_OPF_NOT_PRESENT) {


this will trigger for op = 'call'


>              /* Wrong entry in op definitions? */
>              if (def->used) {
>                  fprintf(stderr, "Invalid op definition for %s\n", def->name);
> diff --git a/tcg/tcg.h b/tcg/tcg.h
> index df375cf..72b694f 100644
> --- a/tcg/tcg.h
> +++ b/tcg/tcg.h
> @@ -593,7 +593,8 @@ enum {
>      TCG_OPF_SIDE_EFFECTS = 0x04,
>      /* Instruction operands are 64-bits (otherwise 32-bits).  */
>      TCG_OPF_64BIT        = 0x08,
> -    /* Instruction is optional and not implemented by the host.  */
> +    /* Instruction is optional and not implemented by the host, or insn
> +       is generic and should not be implemened by the host.  */
>      TCG_OPF_NOT_PRESENT  = 0x10,
>  };
>  
> -- 
> 1.8.1.4