Re: [Qemu-devel] [PATCH for-2.5 08/30] m68k: update CPU flags management

[Laurent Vivier]

Le 12/08/2015 07:12, Richard Henderson a écrit :
> On 08/09/2015 01:13 PM, Laurent Vivier wrote:
>> @@ -798,9 +796,9 @@ void HELPER(mac_set_flags)(CPUM68KState *env,
>> uint32_t acc)
>>       }
>>   }
>>
>> -void HELPER(flush_flags)(CPUM68KState *env, uint32_t cc_op)
>> +uint32_t HELPER(flush_flags)(CPUM68KState *env, uint32_t op)
>>   {
>> -    cpu_m68k_flush_flags(env, cc_op);
>> +    return cpu_m68k_flush_flags(env, op);
>>   }
> 
> Since this function no longer modifies ENV, it probably deserves a
> better name than "flush_flags".  FWIW cc_compute_all isn't a bad name,
> if you're copying i386 anyway...
> 
>> -DEF_HELPER_2(flush_flags, void, env, i32)
>> +DEF_HELPER_2(flush_flags, i32, env, i32)
> 
> Modify to use DEF_HELPER_FLAGS while you're at it.  At the moment it
> reads some globals, but doesn't write any, or have any other side effects.

It writes "env->cc_x", so I guess I can't use DEF_HELPER_FLAGS ?

> 
>>   static inline void gen_flush_flags(DisasContext *s)
>>   {
>>       if (s->cc_op == CC_OP_FLAGS)
>>           return;
>> -    gen_flush_cc_op(s);
>> -    gen_helper_flush_flags(cpu_env, QREG_CC_OP);
>> -    s->cc_op = CC_OP_FLAGS;
>> +    if (s->cc_op == CC_OP_DYNAMIC) {
>> +        gen_helper_flush_flags(QREG_CC_DEST, cpu_env, QREG_CC_OP);
>> +    } else {
>> +        gen_helper_flush_flags(QREG_CC_DEST, cpu_env,
>> tcg_const_i32(s->cc_op));
>> +    }
> 
> That const needs to be freed.

perhaps I'm wrong, what I had understood is:

tcg_const_i32() creates a tcg_temp_new_i32(), and tcg_temp_new_i32() are
automatically freed at end of tcg block (whereas tcg_const_local adn
tcg_temp_local are not).

>> @@ -1248,7 +1294,6 @@ DISAS_INSN(bitop_im)
>>           DEST_EA(env, insn, opsize, tmp, &addr);
>>       }
>>   }
>> -
>>   DISAS_INSN(arith_im)
>>   {
>>       int op;
> 
> Careful with the errant whitespace changes.
> 
>> @@ -1706,16 +1745,18 @@ DISAS_INSN(branch)
>>           /* bsr */
>>           gen_push(s, tcg_const_i32(s->pc));
>>       }
>> -    gen_flush_cc_op(s);
>>       if (op > 1) {
>>           /* Bcc */
>>           l1 = gen_new_label();
>>           gen_jmpcc(s, ((insn >> 8) & 0xf) ^ 1, l1);
>> +        update_cc_op(s);
>>           gen_jmp_tb(s, 1, base + offset);
>>           gen_set_label(l1);
>> +        update_cc_op(s);
>>           gen_jmp_tb(s, 0, s->pc);
> 
> Ideally you'd do this only once, before the jmpcc.
> 
> 
> r~