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

[Richard Henderson]

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~