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

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

[Richard Henderson]

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 <rth@twiddle.net>
---
 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)
 
-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) {
             /* 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

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

[Claudio Fontana]

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