Re: [Qemu-devel] [PATCH v3] target/ppc: Fix carry flag setting for shift algebraic instructions

[David Gibson]

On Thu, Oct 05, 2017 at 08:42:56AM -0400, Richard Henderson wrote:
> On 10/03/2017 02:23 AM, Sandipan Das wrote:
> > @@ -231,6 +231,10 @@ target_ulong helper_sraw(CPUPPCState *env, 
> > target_ulong value,
> >          ret = (int32_t)value >> 31;
> >          env->ca = (ret != 0);
> >      }
> > +
> > +    /* update CA32 for ISA v3.0 */
> > +    env->ca32 = env->ca;
> 
> As I said before, modify ca32 only when ca is modified.
> E.g.
> 
>   env->ca32 = env->ca = (ret != 0);
> 
> > @@ -257,6 +261,10 @@ target_ulong helper_srad(CPUPPCState *env, 
> > target_ulong value,
> >          ret = (int64_t)value >> 63;
> >          env->ca = (ret != 0);
> >      }
> > +
> > +    /* update CA32 for ISA v3.0 */
> > +    env->ca32 = env->ca;
> 
> Likewise.
> 
> > @@ -2192,6 +2192,10 @@ static void gen_srawi(DisasContext *ctx)
> >          tcg_gen_setcondi_tl(TCG_COND_NE, cpu_ca, cpu_ca, 0);
> >          tcg_gen_sari_tl(dst, dst, sh);
> >      }
> > +
> > +    /* update CA32 for ISA v3.0 */
> > +    tcg_gen_mov_tl(cpu_ca32, cpu_ca);
> 
> Likewise.

Also, for the helper functions it definitely makes sense to always set
CA32 when CA is set, regardless of CPU model, it's close enough to
free.  When we're generating code, however, the trade-off is
different, we only need to test the CPU model at translate time, but
we need to execute the generated instrucitons potentially more times.

So I'm wondering if for the gen_* functions we _should_ be checking
for ISA300 before generating the CA32 update instructions.

> 
> > @@ -2269,6 +2273,10 @@ static inline void gen_sradi(DisasContext *ctx, int 
> > n)
> >          tcg_gen_setcondi_tl(TCG_COND_NE, cpu_ca, cpu_ca, 0);
> >          tcg_gen_sari_tl(dst, src, sh);
> >      }
> > +
> > +    /* update CA32 for ISA v3.0 */
> > +    tcg_gen_mov_tl(cpu_ca32, cpu_ca);
> 
> Likewise.
> 
> 
> r~
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature