Alex Bennée writes:
> Lluís Vilanova <vilan...@ac.upc.edu> writes:
>> Signed-off-by: Lluís Vilanova <vilan...@ac.upc.edu>
>> ---
>> accel/tcg/Makefile.objs | 1
>> accel/tcg/translator.c | 152
>> +++++++++++++++++++++++++++++++++++++++++++++
>> include/exec/gen-icount.h | 2 -
>> include/exec/translator.h | 99 +++++++++++++++++++++++++++++
>> 4 files changed, 253 insertions(+), 1 deletion(-)
>> create mode 100644 accel/tcg/translator.c
>>
>> diff --git a/accel/tcg/Makefile.objs b/accel/tcg/Makefile.objs
>> index f173cd5397..3a5da5357c 100644
>> --- a/accel/tcg/Makefile.objs
>> +++ b/accel/tcg/Makefile.objs
>> @@ -1,3 +1,4 @@
>> obj-$(CONFIG_SOFTMMU) += tcg-all.o
>> obj-$(CONFIG_SOFTMMU) += cputlb.o
>> obj-y += cpu-exec.o cpu-exec-common.o translate-all.o translate-common.o
>> +obj-y += translator.o
> There is a merge conflict here with the current master.
I'll rebase for v13.
>> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
>> new file mode 100644
>> index 0000000000..9e0343cbb1
>> --- /dev/null
>> +++ b/accel/tcg/translator.c
>> @@ -0,0 +1,152 @@
>> +/*
>> + * Generic intermediate code generation.
>> + *
>> + * Copyright (C) 2016-2017 Lluís Vilanova <vilan...@ac.upc.edu>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu-common.h"
>> +#include "qemu/error-report.h"
>> +#include "cpu.h"
>> +#include "tcg/tcg.h"
>> +#include "tcg/tcg-op.h"
>> +#include "exec/exec-all.h"
>> +#include "exec/gen-icount.h"
>> +#include "exec/log.h"
>> +#include "exec/translator.h"
>> +
>> +
>> +static inline void translate_block_tcg_check(const DisasContextBase *db)
>> +{
>> + if (tcg_check_temp_count()) {
>> + error_report("warning: TCG temporary leaks before "TARGET_FMT_lx,
>> + db->pc_next);
>> + }
>> +}
>> +
>> +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;
>> + db->pc_next = db->pc_first;
>> + db->is_jmp = DISAS_NEXT;
>> + db->num_insns = 0;
>> + db->singlestep_enabled = cpu->singlestep_enabled;
>> + ops->init_disas_context(db, cpu);
>> +
>> + /* Initialize globals */
>> + tcg_clear_temp_count();
>> +
>> + /* Instruction counting */
>> + max_insns = db->tb->cflags & CF_COUNT_MASK;
>> + if (max_insns == 0) {
>> + max_insns = CF_COUNT_MASK;
>> + }
>> + if (max_insns > TCG_MAX_INSNS) {
>> + max_insns = TCG_MAX_INSNS;
>> + }
>> + if (db->singlestep_enabled || singlestep) {
>> + max_insns = 1;
>> + }
>> +
>> + /* Start translating */
>> + gen_tb_start(db->tb);
>> + ops->tb_start(db, cpu);
>> +
>> + while (true) {
>> + db->num_insns++;
>> + ops->insn_start(db, cpu);
>> +
>> + /* Early exit before breakpoint checks */
>> + if (unlikely(db->is_jmp != DISAS_NEXT)) {
>> + break;
>> + }
>> +
>> + /* Pass breakpoint hits to target for further processing */
>> + if (unlikely(!QTAILQ_EMPTY(&cpu->breakpoints))) {
>> + CPUBreakpoint *bp;
>> + QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
>> + if (bp->pc == db->pc_next) {
>> + BreakpointCheckType bp_check =
>> + ops->breakpoint_check(db, cpu, bp);
>> + switch (bp_check) {
>> + case BC_MISS:
>> + /* Target ignored this breakpoint, go to next */
>> + break;
>> + case BC_HIT_INSN:
>> + /* Hit, keep translating */
>> + /*
>> + * TODO: if we're never going to have more than one
>> + * BP in a single address, we can simply use a
>> + * bool here.
>> + */
>> + goto done_breakpoints;
>> + case BC_HIT_TB:
>> + /* Hit, end TB */
>> + goto done_generating;
>> + default:
>> + g_assert_not_reached();
>> + }
>> + }
>> + }
>> + }
>> + done_breakpoints:
> For the sake of keeping the core loop easy to follow maybe it would be
> better to have a helper function for the breakpoint handling? Really
> there is only one result from the helper which is do we continue the
> loop or jump to done_generating.
The new v13 has a much simpler loop that will hopefully address your concerns.
>> +
>> + /* Accept I/O on last instruction */
>> + if (db->num_insns == max_insns && (db->tb->cflags & CF_LAST_IO)) {
>> + gen_io_start();
>> + }
>> +
>> + /* Disassemble one instruction */
>> + db->pc_next = ops->translate_insn(db, cpu);
>> +
>> + /**************************************************/
>> + /* Conditions to stop translation */
>> + /**************************************************/
>> +
>> + /* Target-specific conditions set by disassembly */
>> + if (db->is_jmp != DISAS_NEXT) {
>> + break;
>> + }
>> +
>> + /* Too many instructions */
>> + if (tcg_op_buf_full() || db->num_insns >= max_insns) {
>> + db->is_jmp = DISAS_TOO_MANY;
>> + break;
>> + }
>> +
>> + translate_block_tcg_check(db);
>> + }
> This may be a personal taste thing but having while(true) {} and breaks
> is harder to follow than do { stuff } while (!done);
I think it is. I prefer to see the loop condition up-front, unless a do-while
makes the condition logic substantially simpler.
Thanks,
Lluis
