Re: [Qemu-devel] [PATCH 02/17] target-openrisc: Streamline arithmetic and OVE

[Richard Henderson <rth@twiddle.net>]

On 09/03/2015 07:16 AM, Bastian Koppelmann wrote:
>
>
> On 09/03/2015 02:17 AM, Richard Henderson wrote:
>> +
>> +void HELPER(ove)(CPUOpenRISCState *env, target_ulong test)
>> +{
>> +    if (unlikely(test) && (env->sr & SR_OVE)) {
>> +        OpenRISCCPU *cpu = openrisc_env_get_cpu(env);
>> +        CPUState *cs = CPU(cpu);
>> +
>> +        cs->exception_index = EXCP_RANGE;
>> +        cpu_restore_state(cs, GETPC());
>> +        cpu_loop_exit(cs);
>> +    }
>> +}
>
> You forgot to check the AECR register, whether the exception really occurs.

The current state of the port suggests it's written to a (pre?) 1.0 
specification, prior to the AECR register being invented.  Let's not try to 
introduce new features just yet.

>> +static void gen_ove_cy(DisasContext *dc, TCGv cy)
>> +{
>> +    gen_helper_ove(cpu_env, cy);
>> +}
>> +
>> +static void gen_ove_ov(DisasContext *dc, TCGv ov)
>> +{
>> +    gen_helper_ove(cpu_env, ov);
>> +}
>> +
>
> Do we really need two functions here? They differ only by the name of the
> second argument. We should directly use gen_helper_ove ().

This is prep work for patch 7/17.

> I do wonder, if we should use TCG globals for sr_cy and sr_ov, as you
> recommended for the TriCore target. It would not change the helper in case of
> no ovf/carry, but simplify addc. And we would not need two deposits for every
> add/sub.

That's patch 7/17.  ;-)

>
>> -            if (ra != 0 && rb != 0) {
>> -                gen_helper_mul32(cpu_R[rd], cpu_env, cpu_R[ra], cpu_R[rb]);
>> -            } else {
>> -                tcg_gen_movi_tl(cpu_R[rd], 0x0);
>> -            }
>> +            gen_mul(dc, cpu_R[rd], cpu_R[ra], cpu_R[rb]);
>
> What happened to this special case here? r0 should always hold zero, so we can
> keep this optimization here, or at least move it to gen_mul().

R0 is not an *architectural* zero.  It's a software convention.  The spec is 
fairly clear on this point.

I agree that there ought to be some special-casing of the software convention, 
but that should require a TB flag to verify the convention is followed.  But 
even then I would not bother retaining the special case here in multiply.  I 
would only use it in the "move" and constant formation types of pseudo 
instructions (or, ori, etc).


r~