Re: [Qemu-devel] [PATCH 01/18] translator: merge max_insns into DisasContextBase

[Michael Clark]

On Sat, Apr 21, 2018 at 6:55 AM, Emilio G. Cota  wrote:

> While at it, use int for both num_insns and max_insns to make
> sure we have same-type comparisons.
>
> Reviewed-by: Richard Henderson 
> Signed-off-by: Emilio G. Cota 
>

Reviewed-by: Michael Clark 


> ---
>  accel/tcg/translator.c | 21 ++---
>  include/exec/translator.h  |  8 
>  target/alpha/translate.c   |  6 ++
>  target/arm/translate-a64.c |  8 +++-
>  target/arm/translate.c |  9 +++--
>  target/hppa/translate.c|  7 ++-
>  target/i386/translate.c|  5 +
>  target/ppc/translate.c |  5 ++---
>  8 files changed, 27 insertions(+), 42 deletions(-)
>
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index 23c6602..0f9dca9 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -34,8 +34,6 @@ void translator_loop_temp_check(DisasContextBase *db)
>  void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
>   CPUState *cpu, TranslationBlock *tb)
>  {
> -int max_insns;
> -
>  /* Initialize DisasContext */
>  db->tb = tb;
>  db->pc_first = tb->pc;
> @@ -45,18 +43,18 @@ void translator_loop(const TranslatorOps *ops,
> DisasContextBase *db,
>  db->singlestep_enabled = cpu->singlestep_enabled;
>
>  /* Instruction counting */
> -max_insns = tb_cflags(db->tb) & CF_COUNT_MASK;
> -if (max_insns == 0) {
> -max_insns = CF_COUNT_MASK;
> +db->max_insns = tb_cflags(db->tb) & CF_COUNT_MASK;
> +if (db->max_insns == 0) {
> +db->max_insns = CF_COUNT_MASK;
>  }
> -if (max_insns > TCG_MAX_INSNS) {
> -max_insns = TCG_MAX_INSNS;
> +if (db->max_insns > TCG_MAX_INSNS) {
> +db->max_insns = TCG_MAX_INSNS;
>  }
>  if (db->singlestep_enabled || singlestep) {
> -max_insns = 1;
> +db->max_insns = 1;
>  }
>
> -max_insns = ops->init_disas_context(db, cpu, max_insns);
> +ops->init_disas_context(db, cpu);
>  tcg_debug_assert(db->is_jmp == DISAS_NEXT);  /* no early exit */
>
>  /* Reset the temp count so that we can identify leaks */
> @@ -95,7 +93,8 @@ void translator_loop(const TranslatorOps *ops,
> DisasContextBase *db,
> update db->pc_next and db->is_jmp to indicate what should be
> done next -- either exiting this loop or locate the start of
> the next instruction.  */
> -if (db->num_insns == max_insns && (tb_cflags(db->tb) &
> CF_LAST_IO)) {
> +if (db->num_insns == db->max_insns
> +&& (tb_cflags(db->tb) & CF_LAST_IO)) {
>  /* Accept I/O on the last instruction.  */
>  gen_io_start();
>  ops->translate_insn(db, cpu);
> @@ -111,7 +110,7 @@ void translator_loop(const TranslatorOps *ops,
> DisasContextBase *db,
>
>  /* Stop translation if the output buffer is full,
> or we have executed all of the allowed instructions.  */
> -if (tcg_op_buf_full() || db->num_insns >= max_insns) {
> +if (tcg_op_buf_full() || db->num_insns >= db->max_insns) {
>  db->is_jmp = DISAS_TOO_MANY;
>  break;
>  }
> diff --git a/include/exec/translator.h b/include/exec/translator.h
> index e2dc2a0..71e7b2c 100644
> --- a/include/exec/translator.h
> +++ b/include/exec/translator.h
> @@ -58,6 +58,7 @@ typedef enum DisasJumpType {
>   *   disassembly).
>   * @is_jmp: What instruction to disassemble next.
>   * @num_insns: Number of translated instructions (including current).
> + * @max_insns: Maximum number of instructions to be translated in this TB.
>   * @singlestep_enabled: "Hardware" single stepping enabled.
>   *
>   * Architecture-agnostic disassembly context.
> @@ -67,7 +68,8 @@ typedef struct DisasContextBase {
>  target_ulong pc_first;
>  target_ulong pc_next;
>  DisasJumpType is_jmp;
> -unsigned int num_insns;
> +int num_insns;
> +int max_insns;
>  bool singlestep_enabled;
>  } DisasContextBase;
>
> @@ -76,7 +78,6 @@ typedef struct DisasContextBase {
>   * @init_disas_context:
>   *  Initialize the target-specific portions of DisasContext struct.
>   *  The generic DisasContextBase has already been initialized.
> - *  Return max_insns, modified as necessary by db->tb->flags.
>   *
>   * @tb_start:
>   *  Emit any code required before the start of the main loop,
> @@ -106,8 +107,7 @@ typedef struct DisasContextBase {
>   *  Print instruction disassembly to log.
>   */
>  typedef struct TranslatorOps {
> -int (*init_disas_context)(DisasContextBase *db, CPUState *cpu,
> -  int max_insns);
> +void (*init_disas_context)(DisasContextBase *db, CPUState *cpu);
>  void (*tb_start)(DisasContextBase *db, CPUState *cpu);
>  void (*insn_start)(DisasContextBase *db, CPUState *cpu);
>  bool 

[Qemu-devel] [PATCH 01/18] translator: merge max_insns into DisasContextBase

[Emilio G. Cota]

While at it, use int for both num_insns and max_insns to make
sure we have same-type comparisons.

Reviewed-by: Richard Henderson 
Signed-off-by: Emilio G. Cota 
---
 accel/tcg/translator.c | 21 ++---
 include/exec/translator.h  |  8 
 target/alpha/translate.c   |  6 ++
 target/arm/translate-a64.c |  8 +++-
 target/arm/translate.c |  9 +++--
 target/hppa/translate.c|  7 ++-
 target/i386/translate.c|  5 +
 target/ppc/translate.c |  5 ++---
 8 files changed, 27 insertions(+), 42 deletions(-)

diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index 23c6602..0f9dca9 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -34,8 +34,6 @@ void translator_loop_temp_check(DisasContextBase *db)
 void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
  CPUState *cpu, TranslationBlock *tb)
 {
-int max_insns;
-
 /* Initialize DisasContext */
 db->tb = tb;
 db->pc_first = tb->pc;
@@ -45,18 +43,18 @@ void translator_loop(const TranslatorOps *ops, 
DisasContextBase *db,
 db->singlestep_enabled = cpu->singlestep_enabled;
 
 /* Instruction counting */
-max_insns = tb_cflags(db->tb) & CF_COUNT_MASK;
-if (max_insns == 0) {
-max_insns = CF_COUNT_MASK;
+db->max_insns = tb_cflags(db->tb) & CF_COUNT_MASK;
+if (db->max_insns == 0) {
+db->max_insns = CF_COUNT_MASK;
 }
-if (max_insns > TCG_MAX_INSNS) {
-max_insns = TCG_MAX_INSNS;
+if (db->max_insns > TCG_MAX_INSNS) {
+db->max_insns = TCG_MAX_INSNS;
 }
 if (db->singlestep_enabled || singlestep) {
-max_insns = 1;
+db->max_insns = 1;
 }
 
-max_insns = ops->init_disas_context(db, cpu, max_insns);
+ops->init_disas_context(db, cpu);
 tcg_debug_assert(db->is_jmp == DISAS_NEXT);  /* no early exit */
 
 /* Reset the temp count so that we can identify leaks */
@@ -95,7 +93,8 @@ void translator_loop(const TranslatorOps *ops, 
DisasContextBase *db,
update db->pc_next and db->is_jmp to indicate what should be
done next -- either exiting this loop or locate the start of
the next instruction.  */
-if (db->num_insns == max_insns && (tb_cflags(db->tb) & CF_LAST_IO)) {
+if (db->num_insns == db->max_insns
+&& (tb_cflags(db->tb) & CF_LAST_IO)) {
 /* Accept I/O on the last instruction.  */
 gen_io_start();
 ops->translate_insn(db, cpu);
@@ -111,7 +110,7 @@ void translator_loop(const TranslatorOps *ops, 
DisasContextBase *db,
 
 /* Stop translation if the output buffer is full,
or we have executed all of the allowed instructions.  */
-if (tcg_op_buf_full() || db->num_insns >= max_insns) {
+if (tcg_op_buf_full() || db->num_insns >= db->max_insns) {
 db->is_jmp = DISAS_TOO_MANY;
 break;
 }
diff --git a/include/exec/translator.h b/include/exec/translator.h
index e2dc2a0..71e7b2c 100644
--- a/include/exec/translator.h
+++ b/include/exec/translator.h
@@ -58,6 +58,7 @@ typedef enum DisasJumpType {
  *   disassembly).
  * @is_jmp: What instruction to disassemble next.
  * @num_insns: Number of translated instructions (including current).
+ * @max_insns: Maximum number of instructions to be translated in this TB.
  * @singlestep_enabled: "Hardware" single stepping enabled.
  *
  * Architecture-agnostic disassembly context.
@@ -67,7 +68,8 @@ typedef struct DisasContextBase {
 target_ulong pc_first;
 target_ulong pc_next;
 DisasJumpType is_jmp;
-unsigned int num_insns;
+int num_insns;
+int max_insns;
 bool singlestep_enabled;
 } DisasContextBase;
 
@@ -76,7 +78,6 @@ typedef struct DisasContextBase {
  * @init_disas_context:
  *  Initialize the target-specific portions of DisasContext struct.
  *  The generic DisasContextBase has already been initialized.
- *  Return max_insns, modified as necessary by db->tb->flags.
  *
  * @tb_start:
  *  Emit any code required before the start of the main loop,
@@ -106,8 +107,7 @@ typedef struct DisasContextBase {
  *  Print instruction disassembly to log.
  */
 typedef struct TranslatorOps {
-int (*init_disas_context)(DisasContextBase *db, CPUState *cpu,
-  int max_insns);
+void (*init_disas_context)(DisasContextBase *db, CPUState *cpu);
 void (*tb_start)(DisasContextBase *db, CPUState *cpu);
 void (*insn_start)(DisasContextBase *db, CPUState *cpu);
 bool (*breakpoint_check)(DisasContextBase *db, CPUState *cpu,
diff --git a/target/alpha/translate.c b/target/alpha/translate.c
index 73a1b5e..15eca71 100644
--- a/target/alpha/translate.c
+++ b/target/alpha/translate.c
@@ -2919,8 +2919,7 @@ static DisasJumpType translate_one(DisasContext *ctx, 
uint32_t insn)
 return ret;
 }
 
-static int