Somehow the original patch set never arrived here. Replying indirectly...
> On Sun, Sep 09, 2012 at 08:19:59PM -0400, crwu...@gmail.com wrote:
>> diff --git a/target-nios2/exec.h b/target-nios2/exec.h
...
>> +static inline int cpu_has_work(CPUState *env)
>> +{
>> + return env->interrupt_request & (CPU_INTERRUPT_HARD |
>> CPU_INTERRUPT_NMI);
>> +}
>> +
>> +static inline int cpu_halted(CPUState *env)
>> +{
>> + if (!env->halted) {
>> + return 0;
>> + }
>> +
>> + if (cpu_has_work(env)) {
>> + env->halted = 0;
>> + return 0;
>> + }
>> +
>> + return EXCP_HALTED;
>> +}
>> +
>> +static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
>> +{
>> + env->regs[R_PC] = tb->pc;
>> +}
Do my eyes deceive me or do you have duplicates of these from cpu.h?
>> +++ b/target-nios2/instruction.c
Any particular reason you split this file out from translate.c?
>> +static void __break(DisasContext *dc, uint32_t code __attribute__((unused)))
Leading un is reserved to the compiler. break1? break_?
>> +/* I-Type instruction */
>> +struct i_type {
>> + uint32_t op:6;
>> + uint32_t imm16:16;
>> + uint32_t b:5;
>> + uint32_t a:5;
>> +} __attribute__((packed));
>> +
>> +union i_type_u {
>> + uint32_t v;
>> + struct i_type i;
>> +};
>> +
>> +#define I_TYPE(instr, op) \
>> + union i_type_u instr_u = { .v = op }; \
>> + struct i_type *instr = &instr_u.i
This is an extremely unportable idea.
Bit field layout differs from big-endian to little-endian, and between
compiler abis. The only reliable method of picking out a specific set
of bits is to shift and mask by hand.
Which you can still do with your I_TYPE/R_TYPE macros (which I do like),
but instead with different structure definitions and initialization.
>> +typedef struct DisasContext {
>> + CPUNios2State *env;
>> + TCGv *cpu_R;
>> + int is_jmp;
>> + target_ulong pc;
>> + struct TranslationBlock *tb;
>> +} DisasContext;
Why are you copying cpu_R here, and using s->cpu_R everywhere?
Why not directly use the global variable cpu_R like everyone else?
I suppose it's related to translate.c vs instruction.c, but I've
already expressed an opinion there...
>> +/* Indirect stringification. Doing two levels allows the parameter to be a
>> + * macro itself. For example, compile with -DFOO=bar, __stringify(FOO)
>> + * converts to "bar".
>> + */
Is there any reason you'd want to do that for the instruction tables?
>> +#define __stringify_1(x...) #x
>> +#define __stringify(x...) __stringify_1(x)
... because there's that leading underscore again, and honestly you don't need
anything but
#define INSTRUCTION(N) { #N, N }
>> +#include "dyngen-exec.h"
This is going away.
>> +void helper_memalign(uint32_t addr, uint32_t dr, uint32_t wr, uint32_t mask)
>> +{
>> + if (addr & mask) {
>> + qemu_log("unaligned access addr=%x mask=%x, wr=%d dr=r%d\n",
>> + addr, mask, wr, dr);
>> + env->regs[CR_BADADDR] = addr;
>> + env->regs[CR_EXCEPTION] = EXCP_UNALIGN << 2;
>> + helper_raise_exception(EXCP_UNALIGN);
>> + }
>> +}
This should be done with
#define ALIGNED_ONLY
directly in the softmmu_template.h helpers. C.f. target-sparc/ldst_helper.c.
>> +uint32_t helper_divs(uint32_t a, uint32_t b)
>> +{
>> + return (int32_t)a / (int32_t)b;
>> +}
>> +
>> +uint32_t helper_divu(uint32_t a, uint32_t b)
>> +{
>> + return a / b;
>> +}
(1) Missing divide by zero check. This will generally put qemu into a loop.
(2) You could (and probably should) use tcg_gen_div{,u}_tl.
I would only suggest external helper functions if you have to check for
additional exceptions apart from X / 0, like -INT_MIN / -1.
>> + /* Dump the CPU state to the log */
>> + if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
>> + qemu_log("--------------\n");
>> + log_cpu_state(env, 0);
>> + }
Don't log cpu state for in_asm. That's a common bug across translators,
and all it does is cause double logging for "-d cpu,in_asm".
>> + LOG_DIS("%8.8x:\t", dc->pc);
Use tcg_gen_debug_insn_start, which makes the tcg opcodes dumps pretty too.
r~
